Add Uint32 representation

Add Uint32 representation.

- Add new representation: kUnboxedUint32.
- Add BinaryUint32Op, UnaryUint32Op, ShiftUint32Op instructions.
- Add new optimization pass which replaces Mint instructions with Uint32 instructions when possible.
- IA32 completed.
- ARM completed.

R=fschneider@google.com, vegorov@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38198

[Cutch, 2014-06-23 15:22:40]

PTAL

[Vyacheslav Egorov (Google), 2014-06-23 20:02:06]

Some preliminary issues:

- & is removed but UnboxInteger(Constant(mask)) is left in the graph which leads
to redundant memory loads where the masking was before, we need to canonicalize
that away.

[potentially canonicalization pass needs rule:

if Unbox*(val) has only environment uses then replace Unbox*(val) with
val->definition() for each unboxing operation.
]

- Do we really want mixed Mint x Uint32 operations? This makes BinaryOp very
complex and also leads to certain in-effeciencies. Maybe we should only have
Mint x Mint and Uint32 x Uint32 operations?

- If result of UnboxInteger operation is floating only into the Uint32 operation
then we should not load anything but th e low word. I look and the generated
code for 

foo(a, b) => (a + b) & 0xFFFFFFFF;

and apparently this is not the case, we load both words and if we run out of
registers we spill useless high word. Look at the code for:

foo(a, b) {
  var x = (a + b) & 0xFFFFFFFF;
  var y = (a - b) & 0xFFFFFFFF;
  return (x == y);
}

- We need Uint32 support in comparisons, but this can wait until next CL, I
guess.

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

https://codereview.chromium.org/345563007/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_ia32.cc:5633: __ movl(out, FieldAddress(value,
Mint::value_offset()));
for constants here it is beneficial to emit immediate move, right now we waste
registers:

0xf66a8ef5    bf792a71f6             mov edi,0xf6712a79  '4294967295'
0xf66a8efa    8b4703                 mov eax,[edi+0x3]

https://codereview.chromium.org/345563007/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_ia32.cc:5837: LocationSummary*
BinaryMintOpInstr::MakeLocationSummary(Isolate* isolate,
This instruction became way too complex. We need to split it into pieces if
possible.

It is also seemingly emitting some strange move:

        ;; v9 <- BinaryMintOp:34(-, v17, v18, v21, v22) [0, 4294967295]
T{not-null, dynamic, Type: class 'int'} 32-BIT
0xf665df00    89d0                   mov eax,edx
0xf665df02    2bc3                   sub eax,ebx

Should not the register allocator take care of copying input only if necessary?
We should have smth like "sub edx, ebx" here

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

https://codereview.chromium.org/345563007/diff/1/runtime/vm/locations.h#newco...
runtime/vm/locations.h:25: kUnboxedMint32,
I would call it kUnboxedUint32. Mint32 is a confusing name.

[Florian Schneider, 2014-06-25 11:36:01]

My suggestion is to start smaller, but with a more generic approach:

1. Instead of simple pattern matching, use a data-flow analysis, and use the
result to eliminate overflow checks for existing mint-operations (BinaryMintOp,
UnaryMintOp and ShiftMintOp). This way you can optimize expressions like (a + b
+ c) & mask right from the start.

2. Next step is to reduce Mint operations to Uint32/Int32 operations. I would
not overload BinaryMintOp to support multiple representations. Instead add
BinaryUint32Op to handle 32-bit unsigned arithmetic.

https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:275: if ((range == NULL) ||
!range->Is32BitMask()) {
This should work for any positive mask value, not just 0xffffffff.

https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:291: if (!target->IsSingleUse()) {
That's too strict: It's ok as long as target is masked with a 32-bit unsigned
integer at all uses.

This also includes for instance stores to Uint8, Uint16, Uint32 typed arrays.
(alternatively, we could make implicit masking for those array stores explicit
in the IL)

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

https://codereview.chromium.org/345563007/diff/20001/runtime/vm/locations.h#n...
runtime/vm/locations.h:25: kUnboxedMint32,
I would rename this to kUnboxedUint32 to more accurately reflect what it is
representing.

[Cutch, 2014-07-07 15:26:51]

On 2014/06/25 11:36:01, Florian Schneider wrote:
> My suggestion is to start smaller, but with a more generic approach:
> 
> 1. Instead of simple pattern matching, use a data-flow analysis, and use the
> result to eliminate overflow checks for existing mint-operations
(BinaryMintOp,
> UnaryMintOp and ShiftMintOp). This way you can optimize expressions like (a +
b
> + c) & mask right from the start.
> 
> 2. Next step is to reduce Mint operations to Uint32/Int32 operations. I would
> not overload BinaryMintOp to support multiple representations. Instead add
> BinaryUint32Op to handle 32-bit unsigned arithmetic.
> 
>
https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
> File runtime/vm/flow_graph_optimizer.cc (right):
> 
>
https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
> runtime/vm/flow_graph_optimizer.cc:275: if ((range == NULL) ||
> !range->Is32BitMask()) {
> This should work for any positive mask value, not just 0xffffffff.
> 
>
https://codereview.chromium.org/345563007/diff/20001/runtime/vm/flow_graph_op...
> runtime/vm/flow_graph_optimizer.cc:291: if (!target->IsSingleUse()) {
> That's too strict: It's ok as long as target is masked with a 32-bit unsigned
> integer at all uses.
> 
> This also includes for instance stores to Uint8, Uint16, Uint32 typed arrays.
> (alternatively, we could make implicit masking for those array stores explicit
> in the IL)
> 
> https://codereview.chromium.org/345563007/diff/20001/runtime/vm/locations.h
> File runtime/vm/locations.h (right):
> 
>
https://codereview.chromium.org/345563007/diff/20001/runtime/vm/locations.h#n...
> runtime/vm/locations.h:25: kUnboxedMint32,
> I would rename this to kUnboxedUint32 to more accurately reflect what it is
> representing.

Hi,

I have taken your feedback into consideration and have uploaded a new approach.

PTAL.

John

[Vyacheslav Egorov (Google), 2014-07-07 16:16:04]

This is certainly going the right way! Awesome. Much simpler and
straightforward.

Please see inline comments: my main concern is that env. uses are not handled
correctly, but otherwise we are getting to the landing shape.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5197: SelectInitialUint32Definitions();
"InitialDefinition" is a confusing name. I would call it
"FindNarrowingOperations" as we are looking for operations that narrow to Uint32
range.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5197: SelectInitialUint32Definitions();
"InitialDefinition" is a confusing name. I would call it
"FindNarrowingOperations" as we are looking for operations that narrow to Uint32
range.

Likewise in all other method names I suggest using Narrowing (or truncating -
which is a name we already use in a couple of places, though I like narrowing
more) to signify what we are looking for.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5242: 
remove empty line

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5245: ?
*block->AsGraphEntry()->initial_definitions()
Did not we already iterate over graph entry above?

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5336: // No uses, skip.
[sidenote: we should DCE those instructions :)]

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5340: Value* use = def->input_use_list();
What about environment uses? It is not safe to ignore them:

class Good {
  use(x) { return x & 0x1; }
}

class Bad {
  use(x) { return x & 0x100000000; }
}

f(o, x) => o.use(x << 1);

main() {
  final good = new Good(), bad = new Bad();
  for (...) f(good, 0x80000000);
  f(bad, 0x80000000);
}

when f() is optimized for Good object then x << 1 can be considered truncating,
but when f() is optimized for Bad it can't.

Fixpointing should take environment uses into account and also take into account
the fact that uint32 operation will not deoptimized if they are converted to
uint32.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5346: only_candidate_uses = false;
return false;

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5352: only_candidate_uses = false;
return false;

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5355: only_candidate_uses =
only_candidate_uses &&
only_candidate_uses are true at this point no need to && it.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5356:
selected_uint32_defs_->Contains(defn->ssa_temp_index());
You can just do:

for (Value* use = def->input_use_list(); use != NULL; use = use->next_use()) {
  Definition* defn = use->instruction()->AsDefinition();
  if ((defn == NULL) ||
      (defn->ssa_temp_index() == -1) ||
      !selected_uint32_defs_->Contains(defn->ssa_temp_index())) {
    return false;
  }
}
return true;

this will cover also previous if (because selcted_uint32_defs_ would not contain
anything but MintDefinitions).

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5359: return only_candidate_uses;
return true;

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5363: void
IntegerInstructionSelector::Iterate() {
I would call it Propagate().

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:9252: HandleBinaryOp(instr, instr->op_kind(),
*instr->left(), *instr->right());
Should result of HandleBinaryOp be truncated to match the semantics of the
Uint32 operations?

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:1332: return CompileType::Int();
On x64 it's actually CompileType::FromCid(kSmiCid) (ditto for the rest).

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:1351: bool
BoxUint32Instr::RecomputeType() {
I am not sure why this is needed given that BoxUint32Instr::ComputeType is
constant.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6035: summary->set_in(1,
Location::RegisterLocation(ECX));
Please support constant operand as well. They are most common.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6052: __ j(ABOVE, deopt);
is there any reason to deopt on shifts that are: 31 < shift? I think we should
only deopt on negative and return 0 on the rest.

[Cutch, 2014-07-07 22:34:29]

PTAL. I've addressed all of your comments except for the renaming ones (will do
that before committing).

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5242: 
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> remove empty line

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5245: ?
*block->AsGraphEntry()->initial_definitions()
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> Did not we already iterate over graph entry above?

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5346: only_candidate_uses = false;
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> return false;

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5352: only_candidate_uses = false;
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> return false;

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5355: only_candidate_uses =
only_candidate_uses &&
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> only_candidate_uses are true at this point no need to && it.

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5356:
selected_uint32_defs_->Contains(defn->ssa_temp_index());
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> You can just do:
> 
> for (Value* use = def->input_use_list(); use != NULL; use = use->next_use()) {
>   Definition* defn = use->instruction()->AsDefinition();
>   if ((defn == NULL) ||
>       (defn->ssa_temp_index() == -1) ||
>       !selected_uint32_defs_->Contains(defn->ssa_temp_index())) {
>     return false;
>   }
> }
> return true;
> 
> this will cover also previous if (because selcted_uint32_defs_ would not
contain
> anything but MintDefinitions).
> 

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:1332: return CompileType::Int();
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> On x64 it's actually CompileType::FromCid(kSmiCid) (ditto for the rest).

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:1351: bool
BoxUint32Instr::RecomputeType() {
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> I am not sure why this is needed given that BoxUint32Instr::ComputeType is
> constant.

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6035: summary->set_in(1,
Location::RegisterLocation(ECX));
On 2014/07/07 16:16:03, Vyacheslav Egorov (Google wrote:
> Please support constant operand as well. They are most common.

Done.

https://codereview.chromium.org/345563007/diff/40001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6052: __ j(ABOVE, deopt);
On 2014/07/07 16:16:04, Vyacheslav Egorov (Google wrote:
> is there any reason to deopt on shifts that are: 31 < shift? I think we should
> only deopt on negative and return 0 on the rest.

Done.

[Florian Schneider, 2014-07-08 12:24:29]

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:458: optimizer.SelectIntegerInstructions();
I think this phase should be run as part of RangeAnalysis by moving it to
RangeAnalysis::Analyze, so that the constraints inserted for branches are still
present.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/deopt_instruc...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/deopt_instruc...
runtime/vm/deopt_instructions.cc:944: intptr_t* source_addr =
maybe uint32_t*?

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5209: if (!def->IsMintDefinition()) {
Add a TODO to handle smi operations as well:

BinarySmiOp can be converted into Uint32 as well. For example when doing n smi
shifts or smi multiplication we currently have to tag the result or the untag
input. These tag/untag operations could be eliminated by doing untagged uint32
arithmetic.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5213: if (def->IsLoadField()) {
Why is this needed? I'd rather enumerate the operations that you want to
consider here (for now *MintOp, Box-/UnboxInteger, etc), since IsMintDefinition
does not do the right thing here (because of e.g. LoadField, but potentially
others as well)

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5225: const GrowableArray<Definition*>&
initial =
Initial definitions are only Parameter or Constant. So I don't think it's
necessary to handle them here.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5245: for (intptr_t i = 0; i <
initial.length(); ++i) {
No need to handle initial definitions here either.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5326: Range* range = op->range();
I think you need to check the range of the input so that the hi-half of the
input is 0. Please also add a test for this case.

[Vyacheslav Egorov (Google), 2014-07-08 12:39:31]

more comments.

please add some tests.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5323: if (def->IsShiftMintOp()) {
should be checking the range of input for the high bits

(1 << 48) >> 32 fits into Uint32 but (1 << 48) can't be truncated to Uint32.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5347: // All environment uses are candidates.
ShiftUint32OpInstr can deopt even though it is truncating/narrowing, but it
throws at this point.

I can't construct the bad case immediately, it is a bit spooky. We need to think
about this.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:9250: HandleBinaryOp(instr, instr->op_kind(),
*instr->left(), *instr->right());
Why there is no truncation here?

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1534: if (IsUint32Mask(this)) {
I would fold it into CanonicalizeCommutativeArithmetic() for generality? If
value is Uint32 and mask is 0x100000000 then result is 0 etc (similar for
addition --- v + 0x100000000 == v).

You can achieve it by supplying truncation rule to
CanonicalizeCommutativeArithmetic, so that instead of == there we compare only
the given number of bits.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6136: __ SmiUntag(shifter);
You can add TODO shift code can be improved if we take shifter range into
account.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6211: __ cmpl(value,
Immediate(kSmiMax));
TODO: take range information into account.

[Cutch, 2014-07-09 17:48:26]

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:458: optimizer.SelectIntegerInstructions();
On 2014/07/08 12:24:28, Florian Schneider wrote:
> I think this phase should be run as part of RangeAnalysis by moving it to
> RangeAnalysis::Analyze, so that the constraints inserted for branches are
still
> present.

Are you suggesting that we do a range analysis pass *after* replacing Mint
operations with Uint32 operations? Please explain more what you're thinking.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/deopt_instruc...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/deopt_instruc...
runtime/vm/deopt_instructions.cc:944: intptr_t* source_addr =
On 2014/07/08 12:24:28, Florian Schneider wrote:
> maybe uint32_t*?

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5209: if (!def->IsMintDefinition()) {
On 2014/07/08 12:24:29, Florian Schneider wrote:
> Add a TODO to handle smi operations as well:
> 
> BinarySmiOp can be converted into Uint32 as well. For example when doing n smi
> shifts or smi multiplication we currently have to tag the result or the untag
> input. These tag/untag operations could be eliminated by doing untagged uint32
> arithmetic.  

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5213: if (def->IsLoadField()) {
On 2014/07/08 12:24:29, Florian Schneider wrote:
> Why is this needed? I'd rather enumerate the operations that you want to
> consider here (for now *MintOp, Box-/UnboxInteger, etc), since
IsMintDefinition
> does not do the right thing here (because of e.g. LoadField, but potentially
> others as well)

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5225: const GrowableArray<Definition*>&
initial =
On 2014/07/08 12:24:28, Florian Schneider wrote:
> Initial definitions are only Parameter or Constant. So I don't think it's
> necessary to handle them here.

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5245: for (intptr_t i = 0; i <
initial.length(); ++i) {
On 2014/07/08 12:24:28, Florian Schneider wrote:
> No need to handle initial definitions here either.

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5323: if (def->IsShiftMintOp()) {
On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> should be checking the range of input for the high bits
> 
> (1 << 48) >> 32 fits into Uint32 but (1 << 48) can't be truncated to Uint32.
>  

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5326: Range* range = op->range();
On 2014/07/08 12:24:29, Florian Schneider wrote:
> I think you need to check the range of the input so that the hi-half of the
> input is 0. Please also add a test for this case.

Done.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5347: // All environment uses are candidates.
On 2014/07/08 12:39:30, Vyacheslav Egorov (Google wrote:
> ShiftUint32OpInstr can deopt even though it is truncating/narrowing, but it
> throws at this point.
> 
> I can't construct the bad case immediately, it is a bit spooky. We need to
think
> about this.

We are conservatively replacing MintOps with Uint32Ops and the Uint32Ops deopt
cases are a subset of the MintOps deopt cases. So, we aren't introducing any new
potential deoptimization points, just possibly removing them. 

BTW- for my own knowledge, I could use some more information/context about
environment uses and the expectations around them.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:9250: HandleBinaryOp(instr, instr->op_kind(),
*instr->left(), *instr->right());
On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> Why there is no truncation here?

Are you saying the constant resulting from HandleBinaryOp should be truncated to
32-bits?

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1534: if (IsUint32Mask(this)) {
On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> I would fold it into CanonicalizeCommutativeArithmetic() for generality? If
> value is Uint32 and mask is 0x100000000 then result is 0 etc (similar for
> addition --- v + 0x100000000 == v).
> 
> You can achieve it by supplying truncation rule to
> CanonicalizeCommutativeArithmetic, so that instead of == there we compare only
> the given number of bits.

This specific case can't be folded into CanonicalizeCommunitativeArithmetic, it
should actually be moved up above the other exit cases.

I've gone ahead and added the truncation value to
CanonicalizeCommunitativeArithmetic and fixed possible crashing bugs (treating a
32 intptr_t as a double!). I should cover the cases you highlighted above.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6211: __ cmpl(value,
Immediate(kSmiMax));
On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> TODO: take range information into account.

Done.

[Florian Schneider, 2014-07-10 13:05:06]

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:458: optimizer.SelectIntegerInstructions();
On 2014/07/09 17:48:25, Cutch wrote:
> On 2014/07/08 12:24:28, Florian Schneider wrote:
> > I think this phase should be run as part of RangeAnalysis by moving it to
> > RangeAnalysis::Analyze, so that the constraints inserted for branches are
> still
> > present.
> 
> Are you suggesting that we do a range analysis pass *after* replacing Mint
> operations with Uint32 operations? Please explain more what you're thinking.

No, your pass runs after range analysis, but before
RangeAnalysis::RemoveConstraints. This way you get better constraints for
branches. e.g.

if (a > 0 && a < 16) {
  // a has range [0..15] here.
}

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5313: for (Value* use =
def->input_use_list(); use != NULL; use = use->next_use()) {
Use Value::Iterator for iterating the use list.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5323: for (Value* use = def->env_use_list();
use != NULL; use = use->next_use()) {
Use Value::Iterator for iterating the use list.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:8111: // Direct access since this instruction
cannot deoptimize, and the deopt-id
This instruction can deoptimize, so the comment is not accurate.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6146: __ j(ABOVE, &zero);
Use range information to eliminate these checkes if possible. You can address
this in a separate CL and add a TODO if you want.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_add_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_add_test.dart:4: // Tests that the VM does not crash on
weird corner cases of class Math.
Update description of this test.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_add_test.dart:13: g(t, x) => t(x + 1);
Also add a test case with larger expressions and check if all additions are
uint32.

e.g. 

(a + b + c + d) & 0xffffffff

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_right_shift_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_right_shift_test.dart:4: // Tests that the VM does not
crash on weird corner cases of class Math.
Update description of this test.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_shift_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_shift_test.dart:4: // Tests that the VM does not crash
on weird corner cases of class Math.
Update description of this test.

[Cutch, 2014-07-10 17:12:54]

ARM port is completed now too.

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:458: optimizer.SelectIntegerInstructions();
On 2014/07/10 13:05:05, Florian Schneider wrote:
> On 2014/07/09 17:48:25, Cutch wrote:
> > On 2014/07/08 12:24:28, Florian Schneider wrote:
> > > I think this phase should be run as part of RangeAnalysis by moving it to
> > > RangeAnalysis::Analyze, so that the constraints inserted for branches are
> > still
> > > present.
> > 
> > Are you suggesting that we do a range analysis pass *after* replacing Mint
> > operations with Uint32 operations? Please explain more what you're thinking.
> 
> No, your pass runs after range analysis, but before
> RangeAnalysis::RemoveConstraints. This way you get better constraints for
> branches. e.g.
> 
> if (a > 0 && a < 16) {
>   // a has range [0..15] here.
> }
> 

I've added a TODO for a follow up CL.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5313: for (Value* use =
def->input_use_list(); use != NULL; use = use->next_use()) {
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Use Value::Iterator for iterating the use list.

Done.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:5323: for (Value* use = def->env_use_list();
use != NULL; use = use->next_use()) {
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Use Value::Iterator for iterating the use list.

Done.

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/345563007/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:6146: __ j(ABOVE, &zero);
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Use range information to eliminate these checkes if possible. You can address
> this in a separate CL and add a TODO if you want.

TODO added.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_add_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_add_test.dart:4: // Tests that the VM does not crash on
weird corner cases of class Math.
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Update description of this test.

Done.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_add_test.dart:13: g(t, x) => t(x + 1);
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Also add a test case with larger expressions and check if all additions are
> uint32.
> 
> e.g. 
> 
> (a + b + c + d) & 0xffffffff

Done.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_right_shift_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_right_shift_test.dart:4: // Tests that the VM does not
crash on weird corner cases of class Math.
On 2014/07/10 13:05:05, Florian Schneider wrote:
> Update description of this test.

Done.

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
File tests/language/vm/uint32_shift_test.dart (right):

https://codereview.chromium.org/345563007/diff/80001/tests/language/vm/uint32...
tests/language/vm/uint32_shift_test.dart:4: // Tests that the VM does not crash
on weird corner cases of class Math.
On 2014/07/10 13:05:06, Florian Schneider wrote:
> Update description of this test.

Done.

[Cutch, 2014-07-10 17:13:20]

+zra for ARM changes.

[Florian Schneider, 2014-07-11 15:33:58]

Lgtm.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:457: // TODO(johnmccutchan): Do integer instruction
selection after
Fine with me. But is there a problem that I don't see with that? 
Because this TODO really only involves moving this one line of code into
RangeAnalysis::Analyze, after InferRanges, before RemoveConstraints.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5347: ASSERT(changed_);
Just make it a local variable:

bool changed = true;

[zra, 2014-07-11 15:45:05]

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6333: // Nothing to do.
Move in to out. They can be different registers. o/w specify in the location
summary that they are the same. If this case isn't actually possible, then merge
with shift_value < 0.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6368: __ CompareImmediate(temp,
kShifterLimit);
With conditional ops:

__ CompareImmediate(temp, kShifterLimit);
__ eor(out, out, Operand(out), HI);
switch (op_kind()) {
  case Token::kSHR:
    __ Lsr(out, left, temp, LS);
    break;
  case Token::kSHL:
    __ Lsl(out, left, temp, LS);
    break;
  default:
    UNREACHABLE();
}

[Vyacheslav Egorov (Google), 2014-07-11 16:00:09]

lgtm

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:9250: HandleBinaryOp(instr, instr->op_kind(),
*instr->left(), *instr->right());
On 2014/07/09 17:48:25, Cutch wrote:
> On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> > Why there is no truncation here?
> 
> Are you saying the constant resulting from HandleBinaryOp should be truncated
to
> 32-bits?

Yes, to uint32 range, to ensure that constant_value computed by constant
propagation would match what instruction would produce when running the same
instruction native code.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5211: return def->IsBoxInteger()   ||   //
BoxMint.
What about Phis? If we have a phi with all operands Uint32 the Phi should become
a UInt32.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5315: while (!it.Done()) {
I think 

for (Value::Iterator it(def->input_use_list());
     !it.Done();
     it.Advance()) {
}

looks nicer. Also you could have a helper function with the actual loop. to do 

return CheckAllUsesNarrowing(def->input_use_list()) &&
CheckAllUsesNarrowing(def->env_use_list);

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1376: } else if (left_value == -1) {
I think you should s/-1/mask/ in this function.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1384: } else if (left_value == -1) {
ditto

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1525: if (IsUint32Mask(this)) {
I think this is not needed anymore once you replace -1 with mask as described
above.

https://codereview.chromium.org/345563007/diff/100001/tests/language/vm/uint3...
File tests/language/vm/uint32_add_test.dart (right):

https://codereview.chromium.org/345563007/diff/100001/tests/language/vm/uint3...
tests/language/vm/uint32_add_test.dart:14: // Foo should be entirely replaced by
Uint32 operations. Running with
note: maybe we should start thinking about writing real IR tests?

[Cutch, 2014-07-11 22:04:14]

PTAL

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/60001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:9250: HandleBinaryOp(instr, instr->op_kind(),
*instr->left(), *instr->right());
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> On 2014/07/09 17:48:25, Cutch wrote:
> > On 2014/07/08 12:39:31, Vyacheslav Egorov (Google wrote:
> > > Why there is no truncation here?
> > 
> > Are you saying the constant resulting from HandleBinaryOp should be
truncated
> to
> > 32-bits?
> 
> Yes, to uint32 range, to ensure that constant_value computed by constant
> propagation would match what instruction would produce when running the same
> instruction native code.

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:457: // TODO(johnmccutchan): Do integer instruction
selection after
On 2014/07/11 15:33:58, Florian Schneider wrote:
> Fine with me. But is there a problem that I don't see with that? 
> Because this TODO really only involves moving this one line of code into
> RangeAnalysis::Analyze, after InferRanges, before RemoveConstraints.

No, it was me not understanding entirely what you were proposing. This is done
now.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5211: return def->IsBoxInteger()   ||   //
BoxMint.
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> What about Phis? If we have a phi with all operands Uint32 the Phi should
become
> a UInt32.

I've added a TODO. We need to improve Phi handling of mints as well. I believe
we currently are always boxing them.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5315: while (!it.Done()) {
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> I think 
> 
> for (Value::Iterator it(def->input_use_list());
>      !it.Done();
>      it.Advance()) {
> }
> 
> looks nicer. Also you could have a helper function with the actual loop. to do

> 
> return CheckAllUsesNarrowing(def->input_use_list()) &&
> CheckAllUsesNarrowing(def->env_use_list);

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5347: ASSERT(changed_);
On 2014/07/11 15:33:58, Florian Schneider wrote:
> Just make it a local variable:
> 
> bool changed = true;

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1376: } else if (left_value == -1) {
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> I think you should s/-1/mask/ in this function.

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1384: } else if (left_value == -1) {
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> ditto

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1525: if (IsUint32Mask(this)) {
On 2014/07/11 16:00:08, Vyacheslav Egorov (Google wrote:
> I think this is not needed anymore once you replace -1 with mask as described
> above.

Done.

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6333: // Nothing to do.
On 2014/07/11 15:45:05, zra wrote:
> Move in to out. They can be different registers. o/w specify in the location
> summary that they are the same. If this case isn't actually possible, then
merge
> with shift_value < 0.

Good catch. I forgot to perform the move in this case. Although I hope no on
ever writes "x = x >> 0;" :)

https://codereview.chromium.org/345563007/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6368: __ CompareImmediate(temp,
kShifterLimit);
On 2014/07/11 15:45:05, zra wrote:
> With conditional ops:
> 
> __ CompareImmediate(temp, kShifterLimit);
> __ eor(out, out, Operand(out), HI);
> switch (op_kind()) {
>   case Token::kSHR:
>     __ Lsr(out, left, temp, LS);
>     break;
>   case Token::kSHL:
>     __ Lsl(out, left, temp, LS);
>     break;
>   default:
>     UNREACHABLE();
> }

Done.

https://codereview.chromium.org/345563007/diff/100001/tests/language/vm/uint3...
File tests/language/vm/uint32_add_test.dart (right):

https://codereview.chromium.org/345563007/diff/100001/tests/language/vm/uint3...
tests/language/vm/uint32_add_test.dart:14: // Foo should be entirely replaced by
Uint32 operations. Running with
On 2014/07/11 16:00:09, Vyacheslav Egorov (Google wrote:
> note: maybe we should start thinking about writing real IR tests?

I would very much like to see us have some IR tests. Have any ideas on how we
could make them easy to write?

[zra, 2014-07-11 22:42:47]

lgtm with small fixes.

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6273: Register left =
locs()->in(0).reg();
const Register here and below

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6333: // Nothing to do.
rm comment

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6496: __ tst(value,
Operand(kSmiTagMask));
Sorry I missed this one before. You can eliminate a branch here.

Label done;
__ tst(value, Operand(kSmiTagMask));
__ mov(out, Operand(value), EQ)
__ SmiUntag(out, EQ);
__ b(&done, EQ);
__ CompareClassId(...);
__ b(deopt, NE);
__ LoadFromOffset(...);
__ Bind(&done);

[Cutch, 2014-07-14 14:57:33]

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/345563007/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6496: __ tst(value,
Operand(kSmiTagMask));
On 2014/07/11 22:42:47, zra wrote:
> Sorry I missed this one before. You can eliminate a branch here.
> 
> Label done;
> __ tst(value, Operand(kSmiTagMask));
> __ mov(out, Operand(value), EQ)
> __ SmiUntag(out, EQ);
> __ b(&done, EQ);
> __ CompareClassId(...);
> __ b(deopt, NE);
> __ LoadFromOffset(...);
> __ Bind(&done);

Done.

[Cutch, 2014-07-14 17:01:20]

Committed patchset #8 manually as r38198 (presubmit successful).