Last Comment Bug 701957 - IonMonkey: Compile JSOP_EQ &  JSOP_NE
: IonMonkey: Compile JSOP_EQ &  JSOP_NE
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: 684381 703791
  Show dependency treegraph
 
Reported: 2011-11-11 17:52 PST by Nicolas B. Pierron [:nbp]
Modified: 2011-11-21 20:42 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int (14.30 KB, patch)
2011-11-17 16:50 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int (18.16 KB, patch)
2011-11-17 18:18 PST, Nicolas B. Pierron [:nbp]
jdemooij: review+
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 17:52:25 PST
Necessary for benchmarks.
Comment 1 Nicolas B. Pierron [:nbp] 2011-11-17 16:50:32 PST
Created attachment 575333 [details] [diff] [review]
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int

This patch is not yet complete on ARM because it does not handle NaNs.
Value & String and any other types would likely be implemented with Bug 679804.
Comment 2 Nicolas B. Pierron [:nbp] 2011-11-17 18:18:44 PST
Created attachment 575356 [details] [diff] [review]
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int

Add VFP Condition flags. (Support for double with NaN checks ?)
Comment 3 Marty Rosenberg [:mjrosenb] 2011-11-18 11:39:03 PST
Comment on attachment 575356 [details] [diff] [review]
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int

Review of attachment 575356 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Assembler-arm.h
@@ +859,5 @@
> +        VFP_GreaterThan = GT,
> +        VFP_LessThanOrEqualOrUnordered = LE,
> +        VFP_LessThanOrEqual = LS,
> +        VFP_LessThanOrUnordered = LT,
> +        VFP_LessThan = CC, // MI is valid too.

There is a trailing comma, some compilers complain about this.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ -931,4 @@
>        case JSOP_LE:
> -        as_cmp(ScratchRegister, O2Reg(ScratchRegister), Overflow);
> -        return Assembler::LessThanOrEqual;
> -        // GT and GE are naturally followed by "and not unordered..."

This seems a bit sketchy, but after talking with Nicolas about it, he's convinced me that it is correct, and it seems to pass all of the tests.
Comment 4 Jan de Mooij [:jandem] (PTO until July 31) 2011-11-19 12:25:48 PST
Comment on attachment 575356 [details] [diff] [review]
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int

Review of attachment 575356 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, most comments are style nits.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +176,5 @@
>      return true;
>  }
>  
>  void
> +CodeGeneratorX86Shared::emitBranch(Assembler::Condition cond, MBasicBlock *mirTrue, MBasicBlock *mirFalse, NaNCond ifNaN)

Nit: in SpiderMonkey code the character limit is 99 characters per line (80 in the rest of Mozilla)

@@ +276,5 @@
> +            Label noNaN;
> +            masm.j(Assembler::NoParity, &noNaN);
> +            masm.movl(Imm32(1), dest);
> +            masm.bind(&noNaN);
> +        }

Can you combine the NaN_isTrue/NaN_isFalse cases, like this:
--
setCC...

if (ifNaN != NaN_unexpected) {
    Label noNaN;
    masm.j(Assembler::NoParity, &noNaN);
    if (ifNaN == NaN_isTrue)
        masm.movl(Imm32(1), dest);
    else
        masm.xorl(dest, dest);
    masm.bind(&noNaN);
}
--
Then you can also get rid of the "Label end" (or move it inside the else block).

@@ +317,5 @@
>      FloatRegister lhs = ToFloatRegister(comp->left());
>      FloatRegister rhs = ToFloatRegister(comp->right());
>  
>      Assembler::Condition cond = masm.compareDoubles(comp->jsop(), lhs, rhs);
> +    NaNCond ifNaN = cond == Assembler::NotEqual ? NaN_isTrue : NaN_isFalse;

Nit: SM style is to put parentheses around the condition, like this:

NaNCond ifNaN = (cond == Assembler::NotEqual) ? NaN_isTrue : NaN_isFalse;

@@ +329,5 @@
>      FloatRegister lhs = ToFloatRegister(comp->left());
>      FloatRegister rhs = ToFloatRegister(comp->right());
>  
>      Assembler::Condition cond = masm.compareDoubles(comp->jsop(), lhs, rhs);
> +    NaNCond ifNaN = cond == Assembler::NotEqual ? NaN_isTrue : NaN_isFalse;

Same here.

::: js/src/jit-test/tests/ion/compareAll.js
@@ +276,5 @@
> +  compareSAndBranch("")
> +];
> +
> +print(result);
> +assertEq(arraysEqual(result, expected), true);

Nice tests.
Comment 5 Jan de Mooij [:jandem] (PTO until July 31) 2011-11-19 13:09:41 PST
I just realized ComparePolicy calls CoercesToDouble, which returns true for MIRType_Undefined. If I understand this correctly, an expression like "undefined === undefined" (= true) will be lowered to "NaN === NaN" (= false). For strict equality we should only coerce int32 to double and never convert the other types (to something other than MIRType_Value).

Another tricky case is:

null == 0 -> false
null <= 0 -> true
Comment 6 David Anderson [:dvander] 2011-11-19 13:19:00 PST
re: comment #5 - nice catch. yeah, here we should specialize but not coerce.

looks good otherwise, drive-by style nit: NaN_IfTrue, etc (capitalize first letters)
Comment 7 Nicolas B. Pierron [:nbp] 2011-11-21 18:40:23 PST
https://hg.mozilla.org/projects/ionmonkey/rev/789acacd0212

(In reply to Jan de Mooij (:jandem) from comment #5)
> I just realized ComparePolicy calls CoercesToDouble, which returns true for
> MIRType_Undefined. If I understand this correctly, an expression like
> "undefined === undefined" (= true) will be lowered to "NaN === NaN" (=
> false). For strict equality we should only coerce int32 to double and never
> convert the other types (to something other than MIRType_Value).
> 
> Another tricky case is:
> 
> null == 0 -> false
> null <= 0 -> true

I'll forward this comment to Bug 679804.
Comment 8 David Anderson [:dvander] 2011-11-21 20:42:26 PST
Wait - is the case in comment #5 broken as-is with this patch? If so, this deserves a separate bug, with a quick plan to fix. We have to be careful about landing known correctness issues.

Note You need to log in before you can comment on or make changes to this bug.