-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Implement UnknownSizeFrame for locals with unknown size #125491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ef0e27c
38e056e
ceb5ce1
359b1e1
9c7abdd
653b229
083efd0
74fdd66
0d0cec3
f00f407
1774253
698fd66
c49fd79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5654,6 +5654,12 @@ void Compiler::generatePatchpointInfo() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsigned varNum = lclNum; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Variable-sized locals reside in a different part of the stack frame. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This brings up OSR support for this kind of stack frame, which I hadn't yet run into. I suppose it's not possible to just skip over these kinds of variables. I would have to either disable OSR for the method, or add some support to allow for copying over the extra frame space as well?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OSR with unknown frame size runs into problems. Particularly how do you address locals from the tier0 frame? You will approximately have a frame that looks like: It is not possible to address the tier0 locals via FP without some non-fixed offset encoding. You will need another frame pointer to do that. We may need to support this eventually, OSR is important for our PGO and tiering strategy. cc @AndyAyersMS, he has thought about this in relation to localloc a lot.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have reserved
This sounds like it needs another pass over the IL, as the earliest we would know
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since OSR is only supported for JIT cases we can probably just compute the size of the "Tier0 vectors/masks" part based on the actual vector/mask size during JIT time. It seems like the most straightforward approach.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To answer your questions:
Note that you cannot move this data around after its initial allocation since there can be pointers pointing to it.
Yes, we currently only support switching very early: runtime/src/coreclr/jit/compiler.cpp Lines 6936 to 6978 in 0c9b431
We haven't even imported the IR at this point, we have only done basic setup of the basic blocks. As part of that we do look at the IL though, but I am not sure how feasible it would be to predict whether we are going to end up with unknown size locals at this point. Perhaps a strategy where we reimported once we saw one and then switched to optimize code would work.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So to recap: when jitting we'll never have unknown sized frames from SVE, and OSR is only needed when jitting, so there is no problem to solve?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can determine the size of the 'unknown' size frame when jitting which should allow us to solve OSR in future. It should be as simple as reading the size of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so. Eventually we will need to solve the "GC pointers at VL-offset dependent locations in the stack frame" problem. That would potentially also make the OSR case a bit more natural since now all fixed size locals are next to each other in the OSR method.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
An idea I've had for this is to add a If this sounds right then it could be a way to start. It would be better to support the frame in OSR, but if this system could be useful more widely then I could give it a try.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't spend time trying to create workarounds. Instead we should just include the size of the "unknown" part in the patchpoint info (maybe it can even be put directly in |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (lvaIsUnknownSizeLocal(varNum)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (gsShadowVarInfo != nullptr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsigned const shadowNum = gsShadowVarInfo[lclNum].shadowCopy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.