Consider nextIntervalRef of other half of double registers for ARM #47032
Conversation
ghost
added
the
area-CodeGen-coreclr
|
@dotnet/jit-contrib |
src/coreclr/jit/lsra.cpp
Outdated
| // Return Value: | ||
| // A RegRecord which forms same double register with regRec | ||
| // | ||
| regNumber LinearScan::findAnotherHalfRegRec(regNumber regNum) |
There was a problem hiding this comment.
This function should be renamed findDoubleRegPair or similar: it shouldn't imply that it's finding a RegRecord.
There was a problem hiding this comment.
Sure. I have changed it to findAnotherHalfRegNum()
| Interval* assignedInterval = physRegs[prevRegOptCandidateRegNum].assignedInterval; | ||
| // The assigned should be non-null, and should have a recentRefPosition, however since | ||
| // this is a heuristic, we don't want a fatal error, so we just assert (not noway_assert). | ||
| bool foundPrevRegOptReg = true; |
There was a problem hiding this comment.
It looks to me like foundPrevRegOptReg will always be true; how can it be set to false?
There was a problem hiding this comment.
Yes, I went back and forth and seems I missed the case when it should be false. I have updated the code.
For reference, I was trying to add back the missing code that was present pre #45135,. You can see it at https://github.com/dotnet/runtime/pull/45135/files#diff-a985c84f41c0c758b9a2f849758cdfd6eac4acf6cfc2151a28d206d70e0358e0L3816-L3824 (need to load diffs for lsra.cpp changes).
BruceForstall
left a comment
BruceForstall
left a comment
There was a problem hiding this comment.
LGTM (if the tests pass)
src/coreclr/jit/lsra.cpp
Outdated
| // None | ||
| // | ||
| // Return Value: | ||
| // A register number which forms same double register with regRec |
There was a problem hiding this comment.
nit: regRec is wrong here.
…r ARM (dotnet#47032)" This reverts commit 247e0ea.
ghost
locked as resolved and limited conversation to collaborators
3 participants
In the recent #45135, we changed the way we go through the heuristics of selecting registers. If we decide to use a busy register, using
FAR_NEXT_REFheuristics, we try to pick a register whose next interval reference is farthest. While calculating the location, we were not taking into consideration the next location of other half of the pair of registers, if the interval type isTYP_DOUBLE. Because of this, we were adding a register in "potential candidate to spill" that doesn't even have assigned interval. Further, inREG_NUMheuristics, we ended up picking this wrong register and hitting assert.The fix was to take into account the next interval ref of other half of double register so we add only the legitimate candidates to be spilled.
Once that was fixed, there was also a problem I discovered in
PREV_REG_OPTheuristics. In this, out of remaining candidates, we check if they have valid assigned interval and if not, we assert because they can't be spill candidates. However, if the current interval isTYP_DOUBLE, we should have also check if the other register of the double register pair has valid interval and if yes, just check the heuristics against its refPosition.Fixes: #46086