[SharedStubs] Port Compare

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(4 attachments)

No description provided.
Assignee: nobody → hv1989
Attachment #8624791 - Flags: review?(jdemooij)
Attachment #8624792 - Flags: review?(jdemooij)
Blocks: 1161516
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.
Attachment #8625933 - Flags: review?(jdemooij)
Attachment #8624791 - Flags: review?(jdemooij) → review+
Comment on attachment 8625933 [details] [diff] [review]
Part 3: Rename Value to Bitwise

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

Nice!
Attachment #8625933 - Flags: review?(jdemooij) → review+
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.
Attachment #8624792 - Flags: review?(jdemooij) → review+
Summary: [MBaselineCache] Port Compare → [SharedStubs] Port Compare
(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.
Apparently these patches regressed Octane badly (according to AWFY).
Flags: needinfo?(hv1989)
Flags: needinfo?(hv1989)
Depends on: 1204675
You need to log in before you can comment on or make changes to this bug.