User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.99 Safari/537.36

This MIR converts a primitive data to the int32 value. To follow the GVN optimization, we can improve MToInt32:foldsTo() to test if the input operand is a constant data and replace the original MIR with the MConstant.

First post a draft here. MDefinition * MToInt32::foldsTo(TempAllocator &alloc) { MDefinition *input = getOperand(0); // Fold this operation is the input operand is constant. if (input->isConstant()) { MConstant *constant = NULL; switch (input->type()) { case MIRType_Null: constant = MConstant::New(alloc, Int32Value(0)); break; case MIRType_Int32: case MIRType_Float32: case MIRType_Double: double dval = input->toNumber(); int32_t ival = static_cast<int32_t>(dval); if (dval == ival) constant = MConstant::New(alloc, Int32Value(ival)); break; } if (constant) return constant; } // Original Snippet. if (input->type() == MIRType_Int32) return input; return this; } And request some comments to continue the process.

Update the draft and the test case will be ready soon. MDefinition * MToInt32::foldsTo(TempAllocator &alloc) { MDefinition *input = getOperand(0); // Fold this operation is the input operand is constant. if (input->isConstant()) { switch (input->type()) { case MIRType_Null: MOZ_ASSERT(conversion == MacroAssembler::IntConversion_Any); return MConstant::New(alloc, Int32Value(0)); case MIRType_Boolean: MOZ_ASSERT(conversion == MacroAssembler::IntConversion_Any); MOZ_ASSERT(conversion == MacroAssembler::IntConversion_NumbersOrBoolsOnly); case MIRType_Int32: Value ival = input->toConstant()->value(); return MConstant::New(alloc, Int32Value(ival.toInt32())); case MIRType_Float32: case MIRType_Double: double dval = input->toNumber(); int32_t ival = static_cast<int32_t>(dval); // Type cast testing. // Only the value within the range of Int32 can be represented as constant. if (dval == ival) return MConstant::New(alloc, Int32Value(ival)); } } // Original Snippet. if (input->type() == MIRType_Int32) return input; return this; }

(In reply to andy.zsshen from comment #3) > case MIRType_Float32: > case MIRType_Double: > double dval = input->toNumber(); > int32_t ival = static_cast<int32_t>(dval); > // Type cast testing. > // Only the value within the range of Int32 can be represented > as constant. > if (dval == ival) > return MConstant::New(alloc, Int32Value(ival)); You want mozilla::NumberEqualsInt32(input->toNumber(), &ival), not an open-coding of it that's buggy in various cases. (I think this is wrong for any float/double outside of int32_t range, but don't quote me.) In general try to use existing methods/operations, not reimplement them.

More formal implementation. And the corresponding test cases are also attached. But I still think for better testing approach.

Comment on attachment 8552826 [details] [diff] [review] MToInt32_FoldsTo.patch Review of attachment 8552826 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please use feedback? if the patch is not complete yet ;) ::: js/src/jit-test/tests/ion/bug1123064.js @@ +12,5 @@ > + // Still thinking for better case. > + str.charAt(1.1); > + > + // Case3: The type of input operand is constant bool. > + // Still thinking. please complete. Out of curiosity: why did you choose not to use ToInteger? I think with ToInteger is possible to test null and boolean ;). ::: js/src/jit/MIR.cpp @@ +2932,5 @@ > + return MConstant::New(alloc, Int32Value(0)); > + case MIRType_Boolean: { > + MOZ_ASSERT(convert == MacroAssembler::IntConversion_Any || > + convert == MacroAssembler::IntConversion_NumbersOrBoolsOnly); > + Value val = input->toConstant()->value(); can you hoist this val outside the switch. This will remove the {} in most cases.

1. Factor out the statment "Value val = input->toConstant()->value();" 2. Add the test cases for int, double, boolean, and null. But for floating point, if the fractional part of the operand is not zero, it cannot pass the checking "NumberEqualsInt32". If the fractional part is zero, it will be represented as MIRType_Int32. Thus I request some advices for better test case.

For double: > function test(a) { > return ToInteger(a); > } > > // Specialize test to take double as input > test(0.1); > test(0.1); > > // Run it with integer, which will get converted to double. > test(0); > test(0); For float: > function test(a) { > var b = Math.fround(a); > return ToInteger(b); > } > > // Specialize test to take double as input > test(0.1); > test(0.1); > > // Run it with integer, which will get converted to double. > test(0); > test(0);

Comment on attachment 8553066 [details] [diff] [review] MToInt32_FoldsTo.patch Review of attachment 8553066 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/bug1123064.js @@ +3,5 @@ > +function toint32() { > + > + // The test case to trigger MToInt32 operation. > + var ToInteger = getSelfHostedValue("ToInteger"); > + Nit: whitespace

These 5 simple cases can hit the corresponding switch handlers of MToInt32::foldsTo().

Comment on attachment 8553195 [details] [diff] [review] MToInt32_FoldsTo.patch Review of attachment 8553195 [details] [diff] [review]: ----------------------------------------------------------------- I suspect this patch is ready? So I did a review ;).

Comment on attachment 8553195 [details] [diff] [review] MToInt32_FoldsTo.patch Review of attachment 8553195 [details] [diff] [review]: ----------------------------------------------------------------- Forgot to remove the feedback request.

(In reply to andy.zsshen from comment #7) > But for floating point, if the fractional part of the operand is not zero, > it cannot pass the checking "NumberEqualsInt32". If the fractional part is > zero, it will be represented as MIRType_Int32. This isn't necessarily true. For asm.js |var f = 0.0;| produces a double, not an int32_t. If that var's never assigned to again, it could easily be treated as a floating point constant, in the right kinds of code. I'm not claiming your patch is *definitely* buggy, because I haven't read it. :-) I'm just suggesting that those assumptions are probably not valid, and so you *probably* have one somewhere. Worth doing a double-check to see if you do, and if you do include a test that would have failed under your hypothesis.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14) > (In reply to andy.zsshen from comment #7) > > But for floating point, if the fractional part of the operand is not zero, > > it cannot pass the checking "NumberEqualsInt32". If the fractional part is > > zero, it will be represented as MIRType_Int32. > > This isn't necessarily true. For asm.js |var f = 0.0;| produces a double, > not an int32_t. If that var's never assigned to again, it could easily be > treated as a floating point constant, in the right kinds of code. I'm not > claiming your patch is *definitely* buggy, because I haven't read it. :-) > I'm just suggesting that those assumptions are probably not valid, and so > you *probably* have one somewhere. Worth doing a double-check to see if you > do, and if you do include a test that would have failed under your > hypothesis. I think it wasn't a general statement, but a statement about MToInt32. For MToInt32 it is save to convert an double/float to int32 (if we don't loose precision). If we do loose precision the MToInt32 will bail and IonMonkey will notice the new type and recompile (normally) without a MToInt32.

And some build fixes https://hg.mozilla.org/integration/mozilla-inbound/rev/341afd35e4ee https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b0706f28ff

https://hg.mozilla.org/mozilla-central/rev/07e3fec694b1 https://hg.mozilla.org/mozilla-central/rev/608f0d854e4f https://hg.mozilla.org/mozilla-central/rev/341afd35e4ee