Closed Bug 1248007 Opened 4 years ago Closed 4 years ago

Refactor LIRGenerator::useBox*

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
For the wasm int64 work (bug 1246648), the LIR will likely end up being similar to what we do for ValueOperand atm (1 register on 64-bit platforms, 2 registers on 32-bit platforms).

The useBox situation is a bit confusing though: we pass (non-box) LAllocation and LDefinition to the constructor, but useBox is used after the LIR instruction has been allocated.

The attached patch adds a new LBoxAllocation class and a useBox() version that returns it. Then we can pass that to the constructor, similar to LAllocation, and don't need the useBox calls afterwards.

Requesting feedback to make sure people agree this is better before I spend time doing this for all LIR instructions. After that, I want to do something similar for int64.
Attachment #8718941 - Flags: feedback?(sunfish)
Attachment #8718941 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8718941 [details] [diff] [review]
WIP

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

Sweet!
Attachment #8718941 - Flags: feedback?(nicolas.b.pierron) → feedback+
Attachment #8718941 - Flags: feedback?(sunfish)
Attached patch PatchSplinter Review
A large patch but most of it is trivial (it was a pain to do this though).
Attachment #8718941 - Attachment is obsolete: true
Attachment #8720764 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8720764 [details] [diff] [review]
Patch

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

Awesome! Thanks.

::: js/src/jit/shared/LIR-shared.h
@@ +2006,4 @@
>      }
>  
>      static const size_t ThisValue = 2;
>      static const size_t NewTarget = 2 + BOX_PIECES;

follow-up:  ThisValue is never used.  Remove the extra room reserved for it.
Attachment #8720764 - Flags: review?(nicolas.b.pierron) → review+
Good catch!
Attachment #8720825 - Flags: review?(nicolas.b.pierron)
Attachment #8720825 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/1b752ce8f7be
https://hg.mozilla.org/mozilla-central/rev/cc1f785b9187
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.