Closed Bug 1507820 Opened 2 years ago Closed 1 year ago

Cranelift: pin HeapReg and maintain it

Categories

(Core :: Javascript: WebAssembly, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

On x64, (ra)baldr excludes r15 from register allocation, and uses it as a pinned register for HeapReg. Cranelift isn't aware of this, and will use r15 for any purpose. There can be bad interactions during tiering because of this: say a main() function is in baseline code, which calls into cranelift code; baseline assumes the callee has preserved r15 and will try to reuse it.

It would be an interesting optimization to have Cranelift use the heap register for heap loads/stores. But in the first place, Cranelift needs to preserve the r15 register.
Attached patch temporary-fix.patch (obsolete) — Splinter Review
Here's a temporary quick fix. I've tried just pushing/popping HeapReg in the right places, but it didn't work for some reason.

With this and other changes from Cranelift 0.24, I could run the demo on webassembly.org \o/
Attached patch heapbase.patchSplinter Review
Works with https://github.com/CraneStation/cranelift/pull/624
Assignee: nobody → bbouvier
Attachment #9030040 - Flags: feedback?
Attachment #9025688 - Attachment is obsolete: true
Attachment #9030040 - Flags: feedback?
Blocks: 1539400

Note, at the moment the only pinned register (in the Ion sense) is HeapReg, which we use on all platforms except x86-32. To load the HeapReg we must first load the TLS, so there's a chained load here if the TLS is not already in a register.

(Of course, the TLS is also a pinned register at the ABI boundary, it is in R14 on x64. But Cranelift handles this as a "special" argument at the moment.)

Priority: -- → P2
Attached patch wip.patch (obsolete) — Splinter Review
Attached patch wip.patch (obsolete) — Splinter Review

New wip patch, passes all tests, to run with a local Cranelift with https://github.com/bnjbvr/cranelift/tree/fixed-ssa-heapreg-wip .

Attachment #9087779 - Attachment is obsolete: true
Attached patch wip.patchSplinter Review
Attachment #9088174 - Attachment is obsolete: true

Latest results on wasm_bench:

# mode=cranelift+cranelift, runs=5, problem size=default
# Lower score is better
benchmark      before after  speedup
box2d		894	850	1.052	[888, 889, 894, 894, 897]	[847, 849, 850, 853, 882]
bullet		1211	1132	1.07	[1210, 1211, 1211, 1214, 1215]	[1126, 1131, 1132, 1132, 1133]
conditionals	345	349	0.989	[336, 337, 345, 346, 347]	[346, 348, 349, 357, 358]
copy		691	690	1.001	[690, 690, 691, 693, 694]	[687, 688, 690, 693, 693]
corrections	931	930	1.001	[928, 929, 931, 937, 948]	[929, 930, 930, 930, 931]
fannkuch	2468	2392	1.032	[2433, 2460, 2468, 2468, 2539]	[2375, 2375, 2392, 2397, 2404]
fasta		811	789	1.028	[809, 811, 811, 813, 813]	[786, 788, 789, 791, 794]
fib		595	518	1.149	[590, 594, 595, 595, 603]	[517, 517, 518, 520, 520]
ifs		192	161	1.193	[183, 184, 192, 194, 194]	[159, 160, 161, 173, 175]
binarytrees	3996	3674	1.088	[3976, 3992, 3996, 4002, 4037]	[3668, 3671, 3674, 3677, 3683]
memops		1483	1421	1.044	[1478, 1479, 1483, 1486, 1494]	[1412, 1421, 1421, 1422, 1424]
primes		990	1012	0.978	[989, 989, 990, 991, 1000]	[1001, 1008, 1012, 1014, 1014]
raybench	1920	1877	1.023	[1915, 1918, 1920, 1920, 1939]	[1871, 1877, 1877, 1886, 1942]
rust-fannkuch	2392	2258	1.059	[2387, 2388, 2392, 2393, 2406]	[2249, 2253, 2258, 2264, 2281]
skinning	683	680	1.004	[680, 681, 683, 683, 691]	[678, 679, 680, 681, 687]
zlib		1271	1237	1.027	[1261, 1266, 1271, 1271, 1274]	[1233, 1234, 1237, 1238, 1249]
Status: NEW → ASSIGNED
Summary: Cranelift doesn't preserve pinned registers → Cranelift: pin HeapReg and maintain it
Depends on: 1580518
Attachment #9091047 - Attachment description: Bug 1507820: Pin HeapReg in Cranelift and use it as the heap base; r?lth → Bug 1507820: Pin HeapReg in Cranelift and use it as the heap base; r=lth
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cbe50166fcf0
Pin HeapReg in Cranelift and use it as the heap base; r=lth
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.