SPMI with JitNoInline=1 asserts in instrumentation code

[SingleAccretion]

Reproduction: run an SPMI replay of the aspnet collection with -jitoption JitNoInline=1.

Expected result: clean replay.

Actual result:

ISSUE: <ASSERT> #1 C:\Users\Accretion\source\dotnet\runtime\src\coreclr\jit\fgprofile.cpp (1709)
Assertion failed 'reinterpret_cast<uint8_t*>(h32->HandleTable) == &m_profileMemory[tableEntry.Offset]'
in 'LoggingConfiguration:Close():this' during 'Profile instrumentation'
(IL size 112; hash 0xe34aa6a7; Instrumented Tier1-OSR)

Preceded by a number of ERROR: AllocPgoInstrumentationBySchema mismatch.

JitNoInline can be a nice setting to verify changes "modulo inlining", it would be nice to keep it assert-free.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Reproduction: run an SPMI replay of the aspnet collection with -jitoption JitNoInline=1.

Expected result: clean replay.

Actual result:

ISSUE: <ASSERT> #1 C:\Users\Accretion\source\dotnet\runtime\src\coreclr\jit\fgprofile.cpp (1709)
Assertion failed 'reinterpret_cast<uint8_t*>(h32->HandleTable) == &m_profileMemory[tableEntry.Offset]'
in 'LoggingConfiguration:Close():this' during 'Profile instrumentation'
(IL size 112; hash 0xe34aa6a7; Instrumented Tier1-OSR)

Preceded by a number of ERROR: AllocPgoInstrumentationBySchema mismatch.

JitNoInline can be a nice setting to verify changes "modulo inlining", it would be nice to keep assert-free.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	8.0.0

[jakobbotsch]

Different inlining decisions in general will cause a different PGO schema to have to be allocated for optimized code with instrumentation.

This seems like a bug in SPMI. allocPgoInstrumentationBySchema should be treated as a compilation result (like ICorJitInfo::allocMem, ICorJitInfo::reserveUnwindInfo, etc.), not as something that must replay correctly.

[BruceForstall]

@jakobbotsch We're getting checked/release asm diffs on x64/arm64 with:

[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: countSchemaItems record 33, replay 31
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [29].InstrumentationKind record 177, replay 65
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [29].ILOffset record 649, replay 656
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [29].Other record -2147483648, replay 0
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [30].InstrumentationKind record 195, replay 65
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [30].ILOffset record 649, replay 666
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [30].Count record 8, replay 1
[23:59:03] ERROR: AllocPgoInstrumentationBySchema mismatch: [30].Other record -2147483648, replay 0
... many more...

https://dev.azure.com/dnceng-public/public/_build/results?buildId=88241&view=results

(if this is a separate problem, we can split this to its own issue)

[jakobbotsch]

@BruceForstall are these actually diffs or "just" failures?

I did see these a couple of weeks ago, but I assumed it was due to recent inliner changes between the time of the collection and the time when superpmi-asmdiffs-checked-release runs (#77600, #77845, #78386). So I assumed it would stabilize again.

However now that I think about it, that should have also caused superpmi-replay to fail. So there may be something related to inliner decisions in checked vs release builds. I will prioritize looking into that.

[jakobbotsch]

Also, as Andy pointed out recently, we wouldn't expect inlining to even alter the schema currently, so something is definitely strange here.

[jakobbotsch]

However now that I think about it, that should have also caused superpmi-replay to fail. So there may be something related to inliner decisions in checked vs release builds. I will prioritize looking into that.

Actually, it seems like -ignoreStoredConfig is what causes the schema allocation to be different. It reproduces with a simple replay when you pass that too.

[jakobbotsch]

The difference is that the particular context sets DOTNET_JitProfileCasts=1, so it does make sense that the schema is different. So we probably just need to implement actual schema layout in SPMI.