Fix GC hole with multi-reg local var stores #65916
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsnull
|
Change dotnet#64857 exposed an existing problem where when generating code for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not enregistered, but the second or subsequent slots was, and those non-first slots contained GC pointers, we wouldn't properly add those GC pointers to the GC tracking sets. This led to cases where the register lifetimes would be killed in the GC info before the actual lifetime was complete. The primary fix is to make `gtHasReg()` handle the `IsMultiRegLclVar()` case. As a side-effect, this fixes some LSRA dumps that weren't displaying multiple registers properly. There are about 50 SPMI asm diffs on win-arm64 where register lifetimes get extended, fixing GC holes. I also made `GetMultiRegCount()` handle the `IsMultiRegLclVar()` case. I made a number of cleanup changes along the way: 1. Fixed two cases of calling `gcInfo.gcMarkRegSetNpt` with regNumber, not regMaskTP 2. Marked some functions `const` 3. Improved some comments 4. Changed "ith" to "i'th" in comments which still doesn't read great, but at least I'm not left trying to parse "ith" as an English word. 5. Use `OperIsScalarLocal()` more broadly 6. Renamed `gtDispRegCount` to `gtDispMultiRegCount` to make it clear it only applies to the multi-reg case. Fixes dotnet#65476.
BruceForstall
force-pushed
the
FixMultiRegGcHole
branch
from
4ada190 to
f7fea2f
Compare
BruceForstall
changed the title
Contributor
|
I will take a look on Monday. |
Contributor
|
/azp run runtime-coreclr superpmi-replay |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
Added a minor nit. I have kicked of superpmi replay to make sure we at least run the jitstressregs.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
Yes, so there are no code size or perfscore diffs, but just the register lifetimes differences? Wonder why asmdiffs (coredistools) is even thinking that there is code difference?
kunalspathak
approved these changes
Feb 28, 2022
Contributor
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
superpmi-replay is clean.
echesakov
reviewed
Mar 1, 2022
| // having a reg if regs are allocated for all its | ||
| // In case of multi-reg call nodes, it is considered having a reg if regs are allocated for ALL its | ||
| // return values. | ||
| // REVIEW: why is this ALL and the other cases are ANY? Explain. |
Contributor
There was a problem hiding this comment.
Do you intend to keep this comment?
Contributor
Author
There was a problem hiding this comment.
Yes, because I don't know the answer :-)
Co-authored-by: Kunal Pathak <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change #64857 exposed an existing problem where when generating code
for a multi-reg GT_STORE_LCL_VAR, if the first register slot was not
enregistered, but the second or subsequent slots was, and those non-first
slots contained GC pointers, we wouldn't properly add those GC pointers
to the GC tracking sets. This led to cases where the register lifetimes
would be killed in the GC info before the actual lifetime was complete.
The primary fix is to make
gtHasReg()handle theIsMultiRegLclVar()case. As a side-effect, this fixes some LSRA dumps that weren't displaying
multiple registers properly.
There are about 50 SPMI asm diffs on win-arm64 where register lifetimes
get extended, fixing GC holes.
I also made
GetMultiRegCount()handle theIsMultiRegLclVar()case.I made a number of cleanup changes along the way:
gcInfo.gcMarkRegSetNptwith regNumber, not regMaskTPconstbut at least I'm not left trying to parse "ith" as an English word.
OperIsScalarLocal()more broadlygtDispRegCounttogtDispMultiRegCountto make it clearit only applies to the multi-reg case.
Fixes #65476.