Fix DEBUG/non-DEBUG SuperPMI asm diffs #76470
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsRecorded SPMI method contexts include configuration environment variables such as Introduce a Contributes to #76347
|
Recorded SPMI method contexts include configuration environment variables such as `COMPlus_JITMinOpts` that are replayed. However, when doing asmdiffs replays to compare a Release to a Checked compiler (non-DEBUG to DEBUG), there may be codegen-altering configuration variables such as JitStress that are only read and interpreted by the DEBUG compiler. This leads to asm diffs. Introduce a `-ignoreStoredConfig` argument to superpmi.exe, and use it in superpmi.py when doing Checked/Release asm diffs, that pretends there are no stored config variables. This assumes that the stored config variables only alter JIT behavior but that they JIT will succeed with or without them. This is also slightly more than necessary: if there is a config variable that the Release compiler knows about, we won't use that, either. However, we have no easy way (currently) to distinguish which variables are DEBUG and which are both DEBUG and non-DEBUG available. Contributes to dotnet#76347
BruceForstall
force-pushed
the
AddSuperpmiIgnoreStoredConfig
branch
from
22b38d2 to
99ebea0
Compare
|
@kunalspathak @jakobbotsch @dotnet/jit-contrib PTAL |
| # If one of the JITs is Release, ignore all the stored configuration variables. | ||
| # This isn't necessary if both are Release builds (and, in fact, it can clear variables | ||
| # that both Release compilers can interpret). However, we rarely or never compare | ||
| # two Release compilers, so this is safest. |
There was a problem hiding this comment.
I was thinking of moving asmdiffs to run on Release instead of Checked.
There was a problem hiding this comment.
When doing so, you'll need to audit usage of diff_with_release. It was introduced for use with Checked/Release diffs (so is not completely accurately named) and also before Release builds had any JitDisasm support (JitDiffableDasm still isn't in Release), so I think it kind of mixes concepts a bit.
| @@ -6903,6 +6904,9 @@ void MethodContext::dmpGetIntConfigValue(const Agnostic_ConfigIntInfo& key, int | |||
|
|
|||
| int MethodContext::repGetIntConfigValue(const WCHAR* name, int defaultValue) | |||
| { | |||
| if (ignoreStoredConfig) | |||
There was a problem hiding this comment.
| if (ignoreStoredConfig) | |
| if (ignoreStoredConfig || (GetIntConfigValue == nullptr)) | |
| return defaultValue; |
There was a problem hiding this comment.
I actually prefer keeping these cases as separate statements.
| @@ -6955,6 +6959,9 @@ void MethodContext::dmpGetStringConfigValue(DWORD nameIndex, DWORD resultIndex) | |||
|
|
|||
| const WCHAR* MethodContext::repGetStringConfigValue(const WCHAR* name) | |||
| { | |||
| if (ignoreStoredConfig) | |||
There was a problem hiding this comment.
| if (ignoreStoredConfig) | |
| if (ignoreStoredConfig || (GetStringConfigValue == nullptr)) | |
| return nullptr; |
Recorded SPMI method contexts include configuration environment variables such as
COMPlus_JITMinOptsthat are replayed. However, when doing asmdiffs replays to compare a Release to a Checked compiler (non-DEBUG to DEBUG), there may be codegen-altering configuration variables such as JitStress that are only read and interpreted by the DEBUG compiler. This leads to asm diffs.Introduce a
-ignoreStoredConfigargument to superpmi.exe, and use it in superpmi.py when doing Checked/Release asm diffs, that pretends there are no stored config variables. This assumes that the stored config variables only alter JIT behavior but that they JIT will succeed with or without them. This is also slightly more than necessary: if there is a config variable that the Release compiler knows about, we won't use that, either. However, we have no easy way (currently) to distinguish which variables are DEBUG and which are both DEBUG and non-DEBUG available.Contributes to #76347