Last Comment Bug 679804 - IM: Implement LCompareV and LCompareVAndBranch
: IM: Implement LCompareV and LCompareVAndBranch
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla12
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: 684381 677774
  Show dependency treegraph
 
Reported: 2011-08-17 11:14 PDT by Andrew Scheff
Modified: 2012-01-30 01:40 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for inbound (7.66 KB, patch)
2012-01-26 07:58 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Patch (20.37 KB, patch)
2012-01-27 07:38 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Andrew Scheff 2011-08-17 11:14:29 PDT
Lowering to LCompareV and LCompareVAndBranch as well as codegen for those operations needs to be done.
Comment 1 Nicolas B. Pierron [:nbp] 2011-11-21 18:41:44 PST
(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] 2012-01-26 06:40:49 PST
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] 2012-01-26 07:58:32 PST
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.
Comment 4 David Anderson [:dvander] 2012-01-26 11:56:47 PST
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] 2012-01-27 06:44:31 PST
(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] 2012-01-27 06:45:33 PST
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] 2012-01-27 07:38:51 PST
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.
Comment 8 Joe Drew (not getting mail) 2012-01-28 19:09:02 PST
https://hg.mozilla.org/mozilla-central/rev/108db1fb5501
Comment 9 Paul Wright 2012-01-28 19:14:58 PST
(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) 2012-01-29 08:49:59 PST
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] 2012-01-30 01:40:34 PST
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.

Note You need to log in before you can comment on or make changes to this bug.