1176288 - [SharedStubs] Port Compare

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Move Compare

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Get Compare working

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 3: Rename Value to Bitwise

In the MIR/LIR we normally use 'V' to indicate we return a value. In most cases that also means a slow VM call.

For compare we have LCompareV, which is actually quite fast! And we have the normal LCompareVM, which does the actual VM. It is actually a bit confusing. So I renamed LCompareV to LCompareBitwise which is what it actually does.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8625933 [details] [diff] [review]
Part 3: Rename Value to Bitwise

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

Nice!

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8624792 [details] [diff] [review]
Part 2: Get Compare working

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

Looks good. Why is the bitwise case treated separately? So you don't have to pass the constraints argument to the MCompare method?

::: js/src/jit/IonBuilder.cpp
@@ +6723,5 @@
> +    MOZ_ASSERT(*emitted == false);
> +
> +    // Try to emit an compare based on the input types.
> +
> +    MCompare::CompareType type = MCompare::possibleCompareType(op, left, right);

Nit: maybe determineCompareType?

::: js/src/jit/MIR.h
@@ +1108,5 @@
> +  public:
> +    // Return if the operands to this instruction are both unsigned.
> +    static bool unsignedOperands(MDefinition* left, MDefinition* right);
> +    bool unsignedOperands();
> +    // Replace any wrapping operands with with the underlying int32 operands

Nit: s/with with/with/ and add an empty line before this comment while you're here.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Comment on attachment 8624792 [details] [diff] [review]
> Part 2: Get Compare working
> 
> Review of attachment 8624792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Why is the bitwise case treated separately? So you don't have to
> pass the constraints argument to the MCompare method?

Multiple reasons. But indeed, I want determineCompareType to be not effectfull. Also some of the helper functions are only available in IonBuilder.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be1b57ffbb74
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb98b244b6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a530b8b339b4

Comment 8

[Guilherme Lima]

Apparently these patches regressed Octane badly (according to AWFY).

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part 4: Fix regression

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7857ca5bfa

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be1b57ffbb74
https://hg.mozilla.org/mozilla-central/rev/9eb98b244b6d
https://hg.mozilla.org/mozilla-central/rev/a530b8b339b4

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e7857ca5bfa