Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Using pinned base registers in Cranelift #1396

Closed
shravanrn opened this issue Feb 19, 2020 · 6 comments
Closed

Using pinned base registers in Cranelift #1396

shravanrn opened this issue Feb 19, 2020 · 6 comments

Comments

@shravanrn
Copy link

shravanrn commented Feb 19, 2020

Feature

I am a PhD student at UCSD investigating a variety of cranelift performance improvements and cranelift Spectre related hardening. So far, these appear very promising and I am hoping to contribute this back to cranelift in the future.

However a key requirement for this work is being able to pin the heap base register in the cranelift compiler. I believe there was support for this earlier, but the feature has since been removed. I am looking for a way to re-enable or re-implement this. I am using cranelift via the Lucet AOT wasm compiler.

Benefit

This will allow for analysis and prototyping of various performance improvements and Spectre related hardening for the cranelift compiler.

Implementation

I believe there was support for this earlier, but this was removed. I am looking for a way to re-enable or re-implement this. A quick and dirty hack for this is fine too, as this is for prototyping, and is not meant for production.

Alternatives

Unfortunately, all of the approaches I am investigating explicitly rely on pinned base registers.

@sunfishcode, @pchickey - We had spoken at the WASM CG about some of this. Would you have thoughts on how best I can proceed here?

@iximeow
Copy link
Collaborator

iximeow commented Feb 19, 2020

So far as I know, Cranelift still supports using a pinned register for the heap base - I just started fixing up a branch in Lucet to expose it as a compilation flag: bytecodealliance/lucet#273

What has you thinking support for this has been removed? If it's missing documentation or something along those lines, we probably ought to fix it :D

edit: @pchickey filled me in - it sounds like the current plan is that the new register allocator/backend doesn't have plans to support this from the start? If that's the case consider this a +1 for still wanting this feature, and quietly 👀 at the conversation here.

@shravanrn
Copy link
Author

@iximeow - thanks for the clarification! Yup, this is indeed a question about both the new and older register allocator. Re the old allocator - I will follow up in the linked lucet PR. Re the new allocator - Yup, I was hoping to start a conversation about supporting this :)

@lars-t-hansen
Copy link
Collaborator

I don't think that we've concluded on this issue - for Firefox, it's really a wasm ABI issue more than a cranelift issue per se. It's believed (based on some preliminary benchmarking) that a pinned heap register is a performance win for many programs, and even when the multi-memory proposal lands I'd be inclined to keep a pinned register for at least one of the memories until we know we're not regressing performance. It's not known how much of win the pinned register is, once we get serious about optimizations that target this particular problem rather than rely on general optimizations & register allocation solutions.

@shravanrn
Copy link
Author

I don't think that we've concluded on this issue... It's not known how much of win the pinned register is...

Ah, gotcha. Thanks for the explanation! Given this, feel free to close this bug as it sounds like discussions on this subject are ongoing.

Just as a summary, the key points I wanted to make from my side was that

  1. Cranelift continues to keep the "pinned & non-spilling" base register support in the current reg allocator
  2. Make the case that Cranelift should continue to support "pinned & non-spilling" base register in the new allocator as well. Our work on security and performance improvements relying on this, while ongoing, have some early results that are promising.

I will try to follow the existing discussions around this feature. Thanks a bunch!

@bnjbvr
Copy link
Member

bnjbvr commented Feb 21, 2020

For what it's worth, the current system allows to pin the heap register via the enable_pinned_reg and use_pinned_reg_as_heap_base settings. Both are false by default. On x86_64, we pin it to r15.

This is definitely something we need to maintain as part of the new regalloc work, as long as Spidermonkey does it, so as to be competitive with Spidermonkey.

@shravanrn
Copy link
Author

Sounds good. Thanks for the info!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants