Closed Bug 1073576 Opened 10 years ago Closed 10 years ago

Optimize strict compares with equal operands

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

We have code like SameValueZero that uses x !== x to check for NaN. We can speed up Array#contains by optimizing x === x and x !== x, where we know the logical results. I.e. the operands aren't NaN.
Attached patch equal-compare-jit (obsolete) — Splinter Review
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8496022 - Flags: review?(hv1989)
Duplicate of Bug 1073575?
Comment on attachment 8496022 [details] [diff] [review]
equal-compare-jit

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

I totally forgot my review queue Monday and Tuesday. That's the reason for my late response, sorry.

- What is the reason "Compare_Value" is not allowed? Compare_Value does a bitwise value compare, which is only allowed for primitives/objects, but disallowed for NaN. As a result this NaN testing isn't an issue for folding.
- Can you add tests for the different compare types doing this NaN check. I think this isn't well tested in current tests.

::: js/src/jit/MIR.cpp
@@ +2836,5 @@
> +        return false;
> +
> +    if (compareType_ == Compare_Value || compareType_ == Compare_Unknown)
> +        return false;
> +

Can you add MOZ_ASSERT(compareType == XXX || compareType == XXX || ...) for all cases that are supported here?
In case a new compareType gets added, we get an assert, hinting that we need to check if this is allowed to fold.
Attachment #8496022 - Flags: review?(hv1989)
Attached patch v2Splinter Review
I have to admit, I didn't really check how Compare_Value works, but it looks like we can't end up with doubles so we are good. I think Compare_Boolean, Compare_Undefined and Compare_Null are fine, because one side needs to be boolean, undefined or null. It would be good if we could get some targeted fuzzing on that pattern (x === x and x !== x) somehow.
Attachment #8496022 - Attachment is obsolete: true
Attachment #8498416 - Flags: review?(hv1989)
Comment on attachment 8498416 [details] [diff] [review]
v2

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

::: js/src/jit-test/tests/basic/strict-compare-same-operands.js
@@ +35,5 @@
> +    assertEq(h !== h, false);
> +    assertEq(i === i, true);
> +    assertEq(i !== i, false);
> +    assertEq(j === j, true);
> +    assertEq(j !== j, false);

This will probably get filtered out in the JS parser or atleast a lot of them.

You will need to create functions that you prepare with the right types:

function undefinedEqualString(a, b) {
    return a === b
}
undefinedEqualString(undefined, "s")
undefinedEqualString(undefined, "t")

function undefinedEqualValue(a, b) {
    return a === b
}
undefinedEqualValue(undefined, "s")
undefinedEqualValue(undefined, false)
undefinedEqualValue(undefined, undefined)
undefinedEqualValue(undefined, "s")
undefinedEqualValue(undefined, false)
undefinedEqualValue(undefined, undefined)

function NaNEqualValue(a, b) {
    return a === b
}
NaNEqualValue(NaN, "s")
NaNEqualValue(NaN, false)
NaNEqualValue(NaN, "s")
NaNEqualValue(NaN, false)

...

Also run them twice. In a for loop or manually like I did here. Since it might detect a type change and get compiled, but not enter yet.

::: js/src/jit/MIR.cpp
@@ +2840,5 @@
> +
> +    switch (compareType_) {
> +      case Compare_Undefined:
> +      case Compare_Null:
> +        MOZ_ASSERT(lhs()->type() != MIRType_Value);

MIRType_Value for lhs() is also good, since it is strict equality with undefined/null

@@ +2848,5 @@
> +      case Compare_Int32MaybeCoerceBoth:
> +      case Compare_Int32MaybeCoerceLHS:
> +      case Compare_Int32MaybeCoerceRHS:
> +      case Compare_UInt32:
> +        MOZ_ASSERT(true);

No need for this MOZ_ASSERT(true);

@@ +2865,5 @@
> +      case Compare_StrictString:
> +        MOZ_ASSERT(lhs()->type() == MIRType_String);
> +        break;
> +      case Compare_Object:
> +        MOZ_ASSERT(true);

No need for this MOZ_ASSERT(true);

@@ +2868,5 @@
> +      case Compare_Object:
> +        MOZ_ASSERT(true);
> +        break;
> +      case Compare_Value:
> +        MOZ_ASSERT(!lhs()->mightBeType(MIRType_Double));

MOZ_ASSERT(!rhs()->mightBeType(MIRType_Double));

@@ +2872,5 @@
> +        MOZ_ASSERT(!lhs()->mightBeType(MIRType_Double));
> +        break;
> +      case Compare_Unknown:
> +      default:
> +        MOZ_ASSERT(false);

you should use MOZ_CRASH() here.

@@ +2874,5 @@
> +      case Compare_Unknown:
> +      default:
> +        MOZ_ASSERT(false);
> +        break;
> +    }

Oh I don't think this gives extra value. I was just fine with doing:
MOZ_ASSERT(compareType_ == Compare_Undefined || compareType == Compare_Null || compareType == Compare_Int32 ...);

Which I would prefer. (But feel free to keep this, if you fix the remarks I had.)
Attachment #8498416 - Flags: review?(hv1989) → review+
> This will probably get filtered out in the JS parser or atleast a lot of them.
I don't think so, I actually checked the MIR to make sure.
>You will need to create functions that you prepare with the right types:
Okay I see. Coming up with a code pattern for all these different compare types is highly annoying.
Oh and I also don't see how your examples would work, because a and b are obviously different, so it would already fail the first check. I am going to land my patch for now and ask the fuzzers if they can target this pattern.
Summary: Optimize compares with equal operands → Optimize strict compares with equal operands
Uhh I screwed up. Backing out.
https://hg.mozilla.org/mozilla-central/rev/e5d631abcd56
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.