1073576 - Optimize strict compares with equal operands

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tom S. (please needinfo tschuster)]

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.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: equal-compare-jit

Comment 2

[Guilherme Lima]

Duplicate of Bug 1073575?

Comment 4

[Hannes Verschore [:h4writer]]

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.

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: v2

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.

Comment 6

[Hannes Verschore [:h4writer]]

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.)

Comment 7

[Tom S. (please needinfo tschuster)]

> 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.

Comment 8

[Tom S. (please needinfo tschuster)]

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.

Comment 9

[Tom S. (please needinfo tschuster)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=87e3705c59e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/1724a6c07316

Comment 10

[Tom S. (please needinfo tschuster)]

Uhh I screwed up. Backing out.

Comment 11

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d631abcd56

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5d631abcd56