Closed
Bug 1404443
Opened 7 years ago
Closed 7 years ago
Baldr: avoid ARM spill/fill in prologue
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file, 2 obsolete files)
15.22 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
I noticed that while it appears ARM doesn't have any non-argument volatile registers, ip/lr are being classified as non-volatile, so we can use them instead of the current spill/fill business.
Assignee | ||
Comment 1•7 years ago
|
||
Unfortunately, lr/ip get clobbered by wasmAssertNonExitInvariants(). We generally have pretty good testing of this invariant, so it seemed simpler just to remove it.
Attachment #8913791 -
Flags: review?(bbouvier)
Comment 2•7 years ago
|
||
Comment on attachment 8913791 [details] [diff] [review] opt-prologue Review of attachment 8913791 [details] [diff] [review]: ----------------------------------------------------------------- The patch is good, but it's actually unfortunate that we lose this assertion: it helps tracking down issues by asserting close to the location where the actual clobbering of FP might have happened. It has nicely helped for bug 1360211. That being said, the push/pop is a bit ugly to have in this code path. How about keeping the assertion, and hiding an (maybe unconditional) push/pop sequence under MacroAssembler::wasmAssertNonExitInvariants? This way, it happens only under #ifdef DEBUG and it's not as disturbing in GenerateCallablePrologue.
Attachment #8913791 -
Flags: review?(bbouvier)
Assignee | ||
Comment 3•7 years ago
|
||
Actually, with bug 1329019 changes, it's now much easier to keep the assert. This patch moves the assert code out of masm and into WasmFrameIter.cpp so it's easier to read it locally.
Attachment #8913791 -
Attachment is obsolete: true
Attachment #8914806 -
Flags: review?(bbouvier)
Assignee | ||
Comment 4•7 years ago
|
||
(forgot to qref)
Attachment #8914806 -
Attachment is obsolete: true
Attachment #8914806 -
Flags: review?(bbouvier)
Attachment #8914807 -
Flags: review?(bbouvier)
Comment 5•7 years ago
|
||
Comment on attachment 8914807 [details] [diff] [review] opt-prologue Review of attachment 8914807 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, even better.
Attachment #8914807 -
Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/46af8801735f Baldr: avoid ARM push/pop in prologue (r=bbouvier)
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46af8801735f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•