Closed Bug 1486857 Opened Last year Closed Last year

IonCompareIC boxes boolean output

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: mgaudet, Assigned: jschulte)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Ted pointed this out during reviews for Bug 1341261. 

This could probably be removed and avoid some box/unbox overhead.
Priority: -- → P3
Attached patch v1.patch (obsolete) — Splinter Review
I took a stab at this, any feedback is welcome.
Attachment #9010786 - Flags: feedback?(mgaudet)
Comment on attachment 9010786 [details] [diff] [review]
v1.patch

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

Strong f+

Looks great, modulo the one style nit below.

::: js/src/jit/Lowering.cpp
@@ +2557,5 @@
> +    MOZ_ASSERT(ins->type() == MIRType::Value || ins->type() == MIRType::Boolean);
> +    LInstruction* lir;
> +    if (ins->type() == MIRType::Value) {
> +        LBinaryValueCache* valueLir = new (alloc())
> +          LBinaryValueCache(useBox(lhs), useBox(rhs), tempFixed(FloatReg0), tempFixed(FloatReg1));

I don't love this line breaking pattern; I'd prefer the breaks to happen after the constructor starts; 

    LBinaryValueCache* valueLir = new (alloc()) LBinaryValueCache(useBox(lhs),
                                                                  useBox(rhs),
                                                                  ....
Attachment #9010786 - Flags: feedback?(mgaudet) → feedback+
Attached patch v2.patch (obsolete) — Splinter Review
Thanks for the quick review!
Assignee: nobody → j_schulte
Attachment #9010786 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9011223 - Flags: review?(mgaudet)
Comment on attachment 9011223 [details] [diff] [review]
v2.patch

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

Still looks good to me. It would probably be wise to do a try build of this before landing; can you do that yourself, or would you prefer I do it?
Attachment #9011223 - Flags: review?(mgaudet) → review+
Depends on: 1494472
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b06faf2f9e
Don't box result of IonCompareIC; r=mgaudet
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/63b06faf2f9e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.