[Wasm Ryujit] Wasm SP is implicitly referenced#124115
[Wasm Ryujit] Wasm SP is implicitly referenced#124115AndyAyersMS wants to merge 1 commit intodotnet:mainfrom
Conversation
Fixes an issue where SP mentions are introduced late (say from a throw helper).
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@SingleAccretion not sure if this is what we agreed on or you have other ideas. Fixes things like where the only SP mention is in the bounds check helper. |
|
fyi @dotnet/jit-contrib |
There was a problem hiding this comment.
Pull request overview
This PR ensures the Wasm shadow stack pointer local (lvaWasmSpArg) is treated as “implicitly referenced” so it won’t be considered unused when references to it are introduced later in compilation (e.g., via late-added helper calls).
Changes:
- Mark the Wasm stack pointer argument local as
lvImplicitlyReferenced. - Mark the reverse-P/Invoke Wasm stack pointer temp local as
lvImplicitlyReferencedwhen it is allocated.
| lvaWasmSpArg = lvaGrabTemp(false DEBUGARG("SP")); | ||
| LclVarDsc* varDsc = lvaGetDesc(lvaWasmSpArg); | ||
| varDsc->lvType = TYP_I_IMPL; | ||
| varDsc->lvImplicitlyReferenced = 1; |
There was a problem hiding this comment.
lvaAllocWasmStackPtr now does lvaGrabTemp(...) followed by manually setting lvImplicitlyReferenced. Since there is an existing helper lvaGrabTempWithImplicitUse(...) (which encapsulates this pattern and is used elsewhere), consider using it here to avoid duplicating the implicit-use bookkeeping and reduce the chance of future drift.
| lvaWasmSpArg = lvaGrabTemp(false DEBUGARG("SP")); | |
| LclVarDsc* varDsc = lvaGetDesc(lvaWasmSpArg); | |
| varDsc->lvType = TYP_I_IMPL; | |
| varDsc->lvImplicitlyReferenced = 1; | |
| lvaWasmSpArg = lvaGrabTempWithImplicitUse(false DEBUGARG("SP")); | |
| LclVarDsc* varDsc = lvaGetDesc(lvaWasmSpArg); | |
| varDsc->lvType = TYP_I_IMPL; |
There was a problem hiding this comment.
It seems like the suggestion here would work, and also it looks like lvaGrabTempWithImplicitUse accounts for inlining compilations?
There was a problem hiding this comment.
It turns out we will never call either lvaAllocWasmStackPtr or lvaInitWasmStackPtr in an inlinee context. Perhaps we should add an assert.
lvaAllocWasmStackPtr will be used to establish a stack pointer local in managed methods that are invoked with a native calling convention (and so have no stack pointer arg). This can only happen in a root method (reverse pinvoke/unmanaged callers only) as inlinees always have a managed calling convention. The prolog of these methods will get the stack pointer global and use that to set up the stack pointer local.
For lvaInitWasmStackPtr the story is a bit more complicated.
In the root method we eagerly create lclvars for all the args and locals. But not so for inlinees. The method that sets up the argument and local lclvars is lvaInitTypeRef. After it does some initial accounting it bails out for inlinees.
Instead, inlinee args and locals generally are handled in one of 3 ways:
(1) if an arg/local is never referred to, it is simply ignored
(2) if an arg is not modifiable in the inlinee, and the caller parameter tree is suitably small and invariant, we try and directly substitute the caller's parameter tree for uses of the arg (see impInlineFetchArg). We call this the "direct sub" optimization.
(3) or, we allocate a new temporary lclvar for use in the inline body (via impInlineFetchLocal or the remaining parts of impInlineFetchArg).
SingleAccretion
left a comment
There was a problem hiding this comment.
I think the ordering issue between the stack level setter and liveness / ref counting still needs to be fixed, but this is also a correct change (since there are implicit references at nodes like LCLHEAP).
Fixes an issue where SP mentions are introduced late (say from a throw helper).