IonMonkey: Compile JSOP_EQ &  JSOP_NE

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Necessary for benchmarks.
(Assignee)

Updated

6 years ago
Assignee: general → nicolas.b.pierron
OS: Linux → All
Hardware: x86_64 → All
Summary: IonMonkey: Compile JSOP_EQ → IonMonkey: Compile JSOP_EQ &  JSOP_NE
(Assignee)

Comment 1

6 years ago
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.
Attachment #575333 - Flags: review?(sstangl)
Attachment #575333 - Flags: review?(jdemooij)
(Assignee)

Comment 2

6 years ago
Created attachment 575356 [details] [diff] [review]
Implement JSOP_{STRICT,}{EQ,NE} for Double & Int

Add VFP Condition flags. (Support for double with NaN checks ?)
Attachment #575333 - Attachment is obsolete: true
Attachment #575333 - Flags: review?(sstangl)
Attachment #575333 - Flags: review?(jdemooij)
Attachment #575356 - Flags: review?(mrosenberg)
Attachment #575356 - Flags: review?(jdemooij)
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.
Attachment #575356 - Flags: review?(mrosenberg) → review+
(Assignee)

Updated

6 years ago
Blocks: 703791
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.
Attachment #575356 - Flags: review?(jdemooij) → review+
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
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)
(Assignee)

Comment 7

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.