function f(y) { return (y > 0) == y; } assertEq(f(0), true); assertEq(f(0), true); assertEq(f(null), false); assertEq(f(null), false); With --ion-eager: a.js:7:0 Error: Assertion failed: got true, expected false The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/3efe3f3d2c25 user: Jan de Mooij date: Wed Jun 19 19:10:04 2013 +0200 summary: Bug 882111 - Don't push an interpreter frame when calling into the JITs. r=djvj

Regression from bug 862103. We specialize the comparison as Compare_Int32, but this doesn't handle |null| correctly, looks like.

Similar to how Compare_Double needs to extra requirements to specify how its operands can be coerced in a generic scenario, Compare_Int32 can't use all coercions permitted by MToInt32.

Comment on attachment 8342580 [details] [diff] [review] patch Review of attachment 8342580 [details] [diff] [review]: ----------------------------------------------------------------- Shu, I think you refactored the convertValueToInt code recently and I have a lot of reviews, would you mind taking this one?

Comment on attachment 8342580 [details] [diff] [review] patch Review of attachment 8342580 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Let me recap what the patch does below to make sure I understand correctly: - Add a new IntConversionInputKind controls whether coercion of input is allowed in MToInt32 - Existing behavior in the IRs that use MToInt32 is unchanged. - MCompare now only allows coercions for comparing int32s if the operands are some combination of int32 and bool. ::: js/src/jit/BaselineInspector.cpp @@ +231,5 @@ > + first->isCompare_Int32WithBoolean() > + ? first->toCompare_Int32WithBoolean() > + : (second && second->isCompare_Int32WithBoolean()) > + ? second->toCompare_Int32WithBoolean() > + : nullptr; Nit: I'd prefer parentheses around hook expressions for better eyeballing. ::: js/src/jit/IonMacroAssembler.h @@ +1186,5 @@ > > + enum IntConversionInputKind { > + IntConversion_NumbersOnly, > + IntConversion_Any > + }; Curious, why did you make this a new type? ::: js/src/jit/MIR.h @@ +3052,5 @@ > public: > INSTRUCTION_HEADER(ToInt32) > + static MToInt32 *New(TempAllocator &alloc, MDefinition *def, > + MacroAssembler::IntConversionInputKind conversion = > + MacroAssembler::IntConversion_Any) Nit: I'm actually not clear on the style here -- should this line be flush with the |MacroAssembler| above?

(In reply to Shu-yu Guo [:shu] from comment #4) > Curious, why did you make this a new type? Which inputs the conversion should tolerate is orthogonal to IntConversionBehavior, which is about what outputs to generate. Adding a new enum seemed better than doubling the number of values in the existing one. https://hg.mozilla.org/integration/mozilla-inbound/rev/70ec3658b113