IM: Implement LCompareV and LCompareVAndBranch

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Andrew Scheff, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla12
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Lowering to LCompareV and LCompareVAndBranch as well as codegen for those operations needs to be done.
(Reporter)

Updated

6 years ago
Blocks: 677774
(Wrote by Jan de Mooij (:jandem) from Bug 701957 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
Blocks: 684381
(Assignee)

Comment 2

5 years ago
Taking. The "unspecialized compare" abort is one of the most common aborts atm. For now I'm going to compile this as a VM call, we can add inline paths later.

One benchmark this affects is Kraken imaging-desaturate, we need JSOP_NOT + this bug to get it to compile.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 591803 [details] [diff] [review]
Patch for inbound

Moves the relational ops to jsinterpinlines. Since x < y is very common in loop conditions, I think it's best to inline these to avoid interpreter regressions.
Attachment #591803 - Flags: review?(dvander)
Comment on attachment 591803 [details] [diff] [review]
Patch for inbound

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

::: js/src/jsinterpinlines.h
@@ +815,5 @@
> +    RELATIONAL_OP(>);
> +}
> +
> +static JS_ALWAYS_INLINE bool
> +GreaterThanOrEqualOperation(JSContext *cx, const Value &lhs, const Value &rhs, bool *res) {

Not sure if you want this bool * to be a JSBool *, compilers can be weird about how bools work which could be annoying for the JIT.
Attachment #591803 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

5 years ago
(In reply to David Anderson [:dvander] from comment #4)
> 
> Not sure if you want this bool * to be a JSBool *, compilers can be weird
> about how bools work which could be annoying for the JIT.

I think it's best to use bool here, and JSBool in the actual function we'll call from JIT code, to avoid having to use JSBool in the interpreter. We'll have to do something similar for js::LooselyEqual/js::StrictlyEqual (they also use bool *).
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/108db1fb5501

Please don't close this bug when merging to m-c, I still have to fix the IonMonkey part.
(Assignee)

Comment 7

5 years ago
Created attachment 592133 [details] [diff] [review]
Patch

- Stubs are in the js::ion namespace to avoid conflicts with the functions in the js namespace. And since these functions have a JSBool outparam, and we're moving from JSBool -> bool in the rest of the VM, I don't think they would be very useful outside js::ion.

- The changes to BinaryV are necessary, since VMFunction's have to be static (the address is used to look up the wrapper). This could cause a weird bug where callVM could call another stub.
Attachment #592133 - Flags: review?(dvander)
Attachment #592133 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/108db1fb5501
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 9

5 years ago
(In reply to Joe Drew (:JOEDREW!) from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/108db1fb5501

See comment 6
Please mark bugs to be left open post-central merge in the whiteboard. I closed something like 75 bugs yesterday post-merge, and I couldn't look through all the comments of all the bugs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/aebead9fb58a

(In reply to Joe Drew (:JOEDREW!) from comment #10)
> Please mark bugs to be left open post-central merge in the whiteboard. I
> closed something like 75 bugs yesterday post-merge, and I couldn't look
> through all the comments of all the bugs.

Makes sense, I will do that next time.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.