Closed
Bug 1070971
Opened 10 years ago
Closed 10 years ago
IonMonkey ARM: declare operands to common ALU and FPU operations to be used at the start.
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: dougc, Assigned: dougc)
Details
Attachments
(1 file, 2 obsolete files)
2.68 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
Declaring operands to be used at the start of the operation allows the register allocator to reuse an argument register for the output which can reduce register pressure. Some of the ALU operations check the arguments after writing to the destination, but do this only in the bail out paths so we can still declare them AtStart for Odin compilation.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → dtc-moz
Comment 2•10 years ago
|
||
Comment on attachment 8493064 [details] [diff] [review] Declare operands to common ALU and FPU operations to be used at the start. Review of attachment 8493064 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/Lowering-arm.cpp @@ +190,5 @@ > LIRGeneratorARM::lowerForALU(LInstructionHelper<1, 2, 0> *ins, MDefinition *mir, MDefinition *lhs, MDefinition *rhs) > { > + // Some operations depend on checking inputs after writing the result, e.g. > + // MulI, but only for bail out paths so for AsmJS we can useAtStart. > + if (gen->compilingAsmJS()) { Checking for AsmJS mode is something we try to avoid when possible. For example, range analysis can also eliminate bailouts in non-AsmJS mode in some cases. Would testing for ins->snapshot() != nullptr work instead? That's still perhaps not perfect, and still worthy of a comment, but it seems like it'd be closer to the minimal requirement.
Assignee | ||
Comment 3•10 years ago
|
||
Thank you for the feedback. Checking for snapshot looks much better, thanks.
Attachment #8493064 -
Attachment is obsolete: true
Attachment #8493483 -
Flags: review?(sunfish)
Comment 4•10 years ago
|
||
Comment on attachment 8493483 [details] [diff] [review] Declare operands to common ALU and FPU operations to be used at the start. Review of attachment 8493483 [details] [diff] [review]: ----------------------------------------------------------------- Architectures with three-address instructions are much simpler than x86 in this territory :-).
Attachment #8493483 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Rebase. Carry forward r+.
Attachment #8493483 -
Attachment is obsolete: true
Attachment #8498853 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eca40ce40fc5
Assignee | ||
Comment 7•10 years ago
|
||
Some try run failures, but they look like infrastructure issues. Requesting this patch land.
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b5b6c55e82
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1b5b6c55e82
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•