Support for unboxed 64-bit integer bitwise operations and equality on ia32.

Support for unboxed 64-bit integer bitwise operations and equality on ia32.

This CL adds AND, OR, XOR and == operations on unboxed 64-bit integers
(aka. mints).

Unboxed mints are stored in xmm registers. Each xmm register location
has an additional bit to keep track of its value representation.

Unboxed mints are materialized on the heap on deoptmization in the same way as
unboxed doubles.

The SSE instructions used are available on all CPUs that support SSE 4.1.
Committed: https://code.google.com/p/dart/source/detail?r=13112

[Florian Schneider, 2012-09-24 13:42:08]

Please have a look. This is working and ready except it is still missing a
fall-back for CPUs that don't support SSE 4.1. I'm going to add that when the
general approach looks good.

[Florian Schneider, 2012-09-24 13:43:38]

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:3555: class BinarySmiOpInstr : public
TemplateDefinition<2> {
This diff looks strange here: I removed BinaryMintOp and added
UnboxedMintBinaryOp. No changes for BinarySmiOp.

[Vyacheslav Egorov (Google), 2012-09-24 14:53:42]

comments

https://codereview.chromium.org/10968059/diff/1/runtime/vm/assembler_ia32_tes...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/assembler_ia32_tes...
runtime/vm/assembler_ia32_test.cc:1673: double res =
reinterpret_cast<OrpdCode>(entry)(12.56e3);
Maybe or two different values?

e.g. -0.0 and 12.56e3 to check that it turns into -12.56e3?

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions.cc
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:150:
Isolate::Current()->DeferMintMaterialization(
You can materialize Smi right away and defer only actual Mint case.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:352:
Isolate::Current()->DeferMintMaterialization(
You can materialize Smi right away and defer only actual Mint case.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/flow_graph_allocat...
File runtime/vm/flow_graph_allocator.h (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/flow_graph_allocat...
runtime/vm/flow_graph_allocator.h:203: BitVector* reps_;
I find name reps_ confusing. Consider:

mint_ranges_ 

or something similiar + add comment.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:654: __ cmpl(temp, Immediate(-1));
you can also use test(temp, temp) and branch on negated condition.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2256:
locs->live_registers()->Remove(locs->temp(1));
I think we don't assigned safepoints for temporary ranges so you can remove this
code.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2286: __ testl(EDX,
Immediate(0xc0000000));
0x0000000100000001 is not a smi but passes this check.

maybe 

mov(temp, eax);
sar(temp, 30);
cmp(temp, edx);

https://codereview.chromium.org/10968059/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/locations.h#newcod...
runtime/vm/locations.h:175: kDouble,
Any reasons to have two different notions of representation (here and in
intermediate language?)

Consider merging them.

[srdjan, 2012-09-24 17:29:24]

Good stuff!

What Slava said and following DBC. 

- Regarding SSE instructions, the VM should bailout if we attempt to run on an
architecture that does not at least support SSE3. For SSE4_1  add
SupportsUnboxedMints() to return false if not supported (we do not want to get
crash reports).

- Please add a flag that can turn off the optimization (maybe also inside
SupportsUnboxedMints)

- Please add tests in tests/language that would demonstrate deoptimization and
optimizations introduced by this code.

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1713: OS::Print("materialing double at %"Px":
%g\n",
s/materialing/materializing/

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:57: int64_t MintXmmRegisterValue(XmmRegister
reg) const {
Alternative possible  name 'XmmRegisterValueAsMint"

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:84: if (from == kTagged && to ==
kUnboxedInteger) {
Please add parenthesis. (here and below).

[Florian Schneider, 2012-09-26 11:51:32]

https://codereview.chromium.org/10968059/diff/1/runtime/vm/assembler_ia32_tes...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/assembler_ia32_tes...
runtime/vm/assembler_ia32_test.cc:1673: double res =
reinterpret_cast<OrpdCode>(entry)(12.56e3);
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> Maybe or two different values?
> 
> e.g. -0.0 and 12.56e3 to check that it turns into -12.56e3?

Done.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions.cc
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:150:
Isolate::Current()->DeferMintMaterialization(
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> You can materialize Smi right away and defer only actual Mint case.

Done.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/deopt_instructions...
runtime/vm/deopt_instructions.cc:352:
Isolate::Current()->DeferMintMaterialization(
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> You can materialize Smi right away and defer only actual Mint case.

Done.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/flow_graph_allocat...
File runtime/vm/flow_graph_allocator.h (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/flow_graph_allocat...
runtime/vm/flow_graph_allocator.h:203: BitVector* reps_;
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> I find name reps_ confusing. Consider:
> 
> mint_ranges_ 
> 
> or something similiar + add comment.

Done.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2256:
locs->live_registers()->Remove(locs->temp(1));
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> I think we don't assigned safepoints for temporary ranges so you can remove
this
> code.

Done.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:2286: __ testl(EDX,
Immediate(0xc0000000));
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> 0x0000000100000001 is not a smi but passes this check.
> 
> maybe 
> 
> mov(temp, eax);
> sar(temp, 30);
> cmp(temp, edx);

Thanks for catching. I'll fix it and add a test.

https://codereview.chromium.org/10968059/diff/1/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://codereview.chromium.org/10968059/diff/1/runtime/vm/locations.h#newcod...
runtime/vm/locations.h:175: kDouble,
On 2012/09/24 14:53:42, Vyacheslav Egorov (Google) wrote:
> Any reasons to have two different notions of representation (here and in
> intermediate language?)
> 
> Consider merging them.

Here I only have 1 bit whereas the representation for SSA values 3 possible
values: tagged, integer and double.

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1713: OS::Print("materialing double at %"Px":
%g\n",
On 2012/09/24 17:29:24, srdjan wrote:
> s/materialing/materializing/

Done.

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:57: int64_t MintXmmRegisterValue(XmmRegister
reg) const {
On 2012/09/24 17:29:24, srdjan wrote:
> Alternative possible  name 'XmmRegisterValueAsMint"

Done.

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/10968059/diff/12004/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:84: if (from == kTagged && to ==
kUnboxedInteger) {
On 2012/09/24 17:29:24, srdjan wrote:
> Please add parenthesis. (here and below).

Done.

[Florian Schneider, 2012-09-27 09:39:52]

Addressed feedback. @srdjan, could you have another look?

Once this is landed, I'll add more 64-bit operations (+, -, *, etc.)

[srdjan, 2012-09-28 16:50:26]

LGTM with comments

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32....
runtime/vm/assembler_ia32.cc:704: void Assembler::orpd(XmmRegister dst,
XmmRegister src) {
Please add asserts at SSE4_1 only instructions.

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32_...
File runtime/vm/assembler_ia32_test.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32_...
runtime/vm/assembler_ia32_test.cc:1658: ASSEMBLER_TEST_GENERATE(Orpd, assembler)
{
Disable appropriate tests if SSE4_1 not supported

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instructi...
File runtime/vm/deopt_instructions.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instructi...
runtime/vm/deopt_instructions.cc:125: class DeoptMintStackSlotInstr : public
DeoptInstr {
Should this be Deopt64bitStackSlotInstr instead since it does not hold a Mint?

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instructi...
File runtime/vm/deopt_instructions.h (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instructi...
runtime/vm/deopt_instructions.h:59: }
Sorry, actually this should be "XmmRegisterValueAsInt64" since you are not
returning a Mint.

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/disassembler_ia...
File runtime/vm/disassembler_ia32.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/disassembler_ia...
runtime/vm/disassembler_ia32.cc:1013: UNIMPLEMENTED();
UNREACHABLE() ?

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_allo...
File runtime/vm/flow_graph_allocator.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_allo...
runtime/vm/flow_graph_allocator.cc:590: instr->representation() ==
kUnboxedInteger) {
Add parenthesis

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_allo...
File runtime/vm/flow_graph_allocator.h (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_allo...
runtime/vm/flow_graph_allocator.h:205: // by SSA temp index.
The comment should say: unboxed Mint representation since it does not maintain
unboxed Smi-s

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_ia32.cc:27: DEFINE_FLAG(bool, unbox_integers,
true, "Optimize 64-bit integer arithmetic.");
Should this be 'unbox_mints' since it does not do anything for unboxed  Smi-s?

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_x64.cc:28: DEFINE_FLAG(bool, unbox_integers,
false, "Optimize 64-bit integer arithmetic.");
Do you need this flag? It will not work on X64 if we enable it, will it? Do you
plan to implement it at all? Maybe just return false in SupportsUnboxedMints.
Maybe we could later have a flag unbox_integers which would unbox mints and
Smi-s.

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.cc:1254: return kIntegerCid;
This type is not very helpful. It would be good if we could be more precise,
saying either kSmiCid or kMintCid. If you can't, then use kDynamicCid.

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language.cc:1264: return kIntegerCid;
ditto. We should use only non-abstract class class-ids here.

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_ia32.cc (right):

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:637: UNREACHABLE();
UNIMPLEMENTED? since you may implement other comparisons in the future? Or
inline that code in EmitUnboxed...

http://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_ia32.cc:2386: // Check if result fits into a
smi.
It is a given in the VM that a Mint object can/should never fit into Smi. Please
add a comment why is this check still necessary (e.g., result of a Mint
operation is not checked to fix into Smi?)

http://codereview.chromium.org/10968059/diff/15001/tests/language/mint_arithm...
File tests/language/mint_arithmetic.dart (right):

http://codereview.chromium.org/10968059/diff/15001/tests/language/mint_arithm...
tests/language/mint_arithmetic.dart:73: test_or_1();
You need add a loop around to trigger optimization. Please run with
--trace-deopt and --trace-compiler to verify the optimized compilation and
deoptimization occurs

[Florian Schneider, 2012-10-02 11:10:31]

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.cc:704: void Assembler::orpd(XmmRegister dst,
XmmRegister src) {
On 2012/09/28 16:50:26, srdjan wrote:
> Please add asserts at SSE4_1 only instructions.

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32...
File runtime/vm/assembler_ia32_test.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32_test.cc:1658: ASSEMBLER_TEST_GENERATE(Orpd, assembler)
{
On 2012/09/28 16:50:26, srdjan wrote:
> Disable appropriate tests if SSE4_1 not supported

I added tests for the 4.1 specific instructions. Those were not tested yet.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.cc:125: class DeoptMintStackSlotInstr : public
DeoptInstr {
On 2012/09/28 16:50:26, srdjan wrote:
> Should this be Deopt64bitStackSlotInstr instead since it does not hold a Mint?

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:59: }
On 2012/09/28 16:50:26, srdjan wrote:
> Sorry, actually this should be "XmmRegisterValueAsInt64" since you are not
> returning a Mint.

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/disassembler_i...
File runtime/vm/disassembler_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/disassembler_i...
runtime/vm/disassembler_ia32.cc:1013: UNIMPLEMENTED();
On 2012/09/28 16:50:26, srdjan wrote:
> UNREACHABLE() ?

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_all...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.cc:590: instr->representation() ==
kUnboxedInteger) {
On 2012/09/28 16:50:26, srdjan wrote:
> Add parenthesis

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_all...
File runtime/vm/flow_graph_allocator.h (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_all...
runtime/vm/flow_graph_allocator.h:205: // by SSA temp index.
On 2012/09/28 16:50:26, srdjan wrote:
> The comment should say: unboxed Mint representation since it does not maintain
> unboxed Smi-s

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:27: DEFINE_FLAG(bool, unbox_integers,
true, "Optimize 64-bit integer arithmetic.");
On 2012/09/28 16:50:26, srdjan wrote:
> Should this be 'unbox_mints' since it does not do anything for unboxed  Smi-s?

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:28: DEFINE_FLAG(bool, unbox_integers,
false, "Optimize 64-bit integer arithmetic.");
On 2012/09/28 16:50:26, srdjan wrote:
> Do you need this flag? It will not work on X64 if we enable it, will it? Do
you
> plan to implement it at all? Maybe just return false in SupportsUnboxedMints.
> Maybe we could later have a flag unbox_integers which would unbox mints and
> Smi-s.

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1254: return kIntegerCid;
On 2012/09/28 16:50:26, srdjan wrote:
> This type is not very helpful. It would be good if we could be more precise,
> saying either kSmiCid or kMintCid. If you can't, then use kDynamicCid.

Done. If we don't know anything about the range of the value, it can be either
smi or mint.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1264: return kIntegerCid;
On 2012/09/28 16:50:26, srdjan wrote:
> ditto. We should use only non-abstract class class-ids here. 

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:637: UNREACHABLE();
On 2012/09/28 16:50:26, srdjan wrote:
> UNIMPLEMENTED? since you may implement other comparisons in the future? Or
> inline that code in EmitUnboxed... 

Done.

https://codereview.chromium.org/10968059/diff/15001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:2386: // Check if result fits into a
smi.
On 2012/09/28 16:50:26, srdjan wrote:
> It is a given in the VM that a Mint object can/should never fit into Smi.
Please
> add a comment why is this check still necessary (e.g., result of a Mint
> operation is not checked to fix into Smi?)

Done.

https://codereview.chromium.org/10968059/diff/15001/tests/language/mint_arith...
File tests/language/mint_arithmetic.dart (right):

https://codereview.chromium.org/10968059/diff/15001/tests/language/mint_arith...
tests/language/mint_arithmetic.dart:73: test_or_1();
On 2012/09/28 16:50:26, srdjan wrote:
> You need add a loop around to trigger optimization. Please run with
> --trace-deopt and --trace-compiler to verify the optimized compilation and
> deoptimization occurs

Done. There are already loops inside each test function. I'm adding one here
too.