Closed Bug 1404443 Opened 7 years ago Closed 7 years ago

Baldr: avoid ARM spill/fill in prologue

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch opt-prologue (obsolete) — Splinter Review
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 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)
Attached patch opt-prologue (obsolete) — Splinter Review
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)
Attached patch opt-prologueSplinter Review
(forgot to qref)
Attachment #8914806 - Attachment is obsolete: true
Attachment #8914806 - Flags: review?(bbouvier)
Attachment #8914807 - Flags: review?(bbouvier)
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)
https://hg.mozilla.org/mozilla-central/rev/46af8801735f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: