Fix majority of failures in libraries-jitstressregs after EHWriteThru enabled #48829
Conversation
When we verify final allocation, we were not resetting the `assignedInterval` if the interval got assigned to a different register. This was not consistent to what we do during allocation. Fixed following failures: https://dev.azure.com/dnceng/public/_build/results?buildId=989595&view=ms.vss-test-web.build-test-results-tab&runId=30998546&resultId=169282&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-5f85fedec0cd4ff9a0/System.Net.Security.Tests/console.73e69194.log?sv=2019-07-07&se=2021-03-03T08%3A52%3A21Z&sr=c&sp=rl&sig=naQOrM%2BszKDyDAO8m%2BU%2Fu149f4cjodTk1dYCKl9cvKs%3D Failures happen on arch/OS - Linux arm64 - Linux arm - Linux x64 - Windows x86 - Windows x64
ghost
added
the
area-CodeGen-coreclr
|
@dotnet/jit-contrib |
src/coreclr/jit/lsra.cpp
Outdated
| // Free Registers. | ||
| // We could use the freeRegisters() method, but we'd have to carefully manage the active intervals. | ||
| for (regNumber reg = REG_FIRST; reg < ACTUAL_REG_COUNT; reg = REG_NEXT(reg)) | ||
| for (regNumber reg = REG_FIRST; reg < ACTUAL_REG_COUNT && regsToFree != RBM_NONE; reg = REG_NEXT(reg)) |
There was a problem hiding this comment.
This new clause is a loop invariant, so maybe hoist it out...?
| for (regNumber reg = REG_FIRST; reg < ACTUAL_REG_COUNT && regsToFree != RBM_NONE; reg = REG_NEXT(reg)) | |
| if (regsToFree != RBM_NONE) | |
| { | |
| for (regNumber reg = REG_FIRST; reg < ACTUAL_REG_COUNT; reg = REG_NEXT(reg)) |
Or perhaps better yet (but maybe more involved) only iterate over the set bits of regsToFree using find first bit or similar tech (presumably regsToFree is mostly zeros so iterating over each bit is a little wasteful).
Also a few lines below the code seems odd, we should make sure we know which of the assignments is the right one.
regsToFree = delayRegsToFree;
regsToFree = RBM_NONE;
There was a problem hiding this comment.
Thanks for pointing it out about the regsToFree = delayRegsToFree; , I should have noticed it. But I did notice other problem where delayRegsToFree in this method is never assigned and is always RBM_NONE so the odd code doesn't do any harm. Probably, some day, I should check right way to introduce delayRegsToFree, but for now, I will fix the odd code as well as get rid of delayRegsToFree variable from this method.
There was a problem hiding this comment.
Also, regsToFree is never used and it was that way from initial commit (6 years back). So as a cleanup, I will also get rid of regsFree and investigate what is the right way as part of #48837.
These two variables were always set to `RBM_NONE`. As part of dotnet#48837, investigate how to reintroduce them.
|
@AndyAyersMS or @BruceForstall - can you please review this? |
|
Were you planning to make changes based on my earlier comments, or leave that for a subsequent PR? |
Ah, sorry, I didn't push it. Let me do it. |
When we verify final allocation, we were not resetting the
assignedIntervalif the interval got assigned to a different register. This was not consistent to what we do during allocation.Fixed following failures:
https://dev.azure.com/dnceng/public/_build/results?buildId=989595&view=ms.vss-test-web.build-test-results-tab&runId=30998546&resultId=169282&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-5f85fedec0cd4ff9a0/System.Net.Security.Tests/console.73e69194.log?sv=2019-07-07&se=2021-03-03T08%3A52%3A21Z&sr=c&sp=rl&sig=naQOrM%2BszKDyDAO8m%2BU%2Fu149f4cjodTk1dYCKl9cvKs%3D
Failures are present on arch/OS: