Closed Bug 1415931 Opened 3 years ago Closed 3 years ago

[MIPS] Make long jumps visible to Wasm module code linking and caching

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Steps to reproduce:

Run jit_test.py   dist/bin/js wasm/spec/skip-stack-guard-page.wast.js on mips32.


Actual results:

Tests failing.


Expected results:

Tests passing.
Attached patch bug1415931.diffSplinter Review
On MIPS implementation local pc-relative branches are expanded into code pattern which includes an absolute address jump (long jump in MacroAssembler) when their range exceeds ± 128 KiB. These jumps where internal to MacroAssembler, only patched during MacroAssembler::executableCopy and otherwise where not known to wasm's module generator and code cache. This patch threats long jumps as CodeLabels that need to be patched making them visible to wasm module generator. It also includes an minor deadcode cleanup for MacroAssembler::backedgeJump.
Attachment #8926941 - Flags: review?(bbouvier)
Component: General → JavaScript Engine: JIT
Assignee: nobody → dragan.mladjenovic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Thanks for the patch! Aren't all these functions also used by the regular JIT (Ion/Baseline) and thus this would break jit support?
Flags: needinfo?(dragan.mladjenovic)
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Thanks for the patch! Aren't all these functions also used by the regular
> JIT (Ion/Baseline) and thus this would break jit support?

Yes, they are used by Ion/Baseline and both of those cases are handled by JitCode::copyFrom(MacroAssembler&) which calls MacroAssembler::processCodeLabels following MacroAssembler::executableCopy. So in that regard we just moved the processing of long jump "code labels" from the later to the former.
(In reply to Dragan Mladjenovic from comment #3)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> > Thanks for the patch! Aren't all these functions also used by the regular
> > JIT (Ion/Baseline) and thus this would break jit support?
> 
> Yes, they are used by Ion/Baseline and both of those cases are handled by
> JitCode::copyFrom(MacroAssembler&) which calls
> MacroAssembler::processCodeLabels following MacroAssembler::executableCopy.
> So in that regard we just moved the processing of long jump "code labels"
> from the later to the former.

Makes sense, I missed that, thank you!
Flags: needinfo?(dragan.mladjenovic)
Comment on attachment 8926941 [details] [diff] [review]
bug1415931.diff

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

OK, then looks good to me, thank you for the patch!
Attachment #8926941 - Flags: review?(bbouvier) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afbde1e5a67d
[MIPS] Make long jumps visible to Wasm module code linking and caching. r=bbouvier
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/afbde1e5a67d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.