679804 - IM: Implement LCompareV and LCompareVAndBranch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Scheff]

Lowering to LCompareV and LCompareVAndBranch as well as codegen for those operations needs to be done.

Comment 1

[Nicolas B. Pierron [:nbp]]

(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

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 5

[Jan de Mooij [:jandem]]

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

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Jan de Mooij [:jandem]]

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

Comment 8

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/108db1fb5501

Comment 9

[Paul Wright]

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

See comment 6

Comment 10

[Joe Drew (not getting mail)]

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.

Comment 11

[Jan de Mooij [:jandem]]

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.