[RyuJIT] Bad CodeGen with Dynamic PGO + JitStressRegs=2 on .NET 8 (x64)

[lhr1101]

Description

I discovered a JIT CodeGen bug using automated differential fuzzing. The program produces different outputs (checksums) depending on whether Dynamic PGO and JitStressRegs are enabled.

Reproduction Steps

1. Create a new .NET 8 console app: dotnet new console -n ReproBug
2. Important: Modify ReproBug.csproj to allow unsafe blocks (required for the checksum function):

   <PropertyGroup>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
   

Expected behavior

set DOTNET_TieredCompilation=0
set DOTNET_TieredPGO=0
set COMPlus_JitStressRegs=0
dotnet run -c Release

The checksum should be 49B5F8F053C8A9CC regardless of PGO/Stress settings.

Actual behavior

set DOTNET_TieredCompilation=1
set DOTNET_TieredPGO=1
set COMPlus_JitStressRegs=2
set DOTNET_EnableAVX2=0
dotnet run -c Release

When Dynamic PGO is enabled alongside JitStressRegs=2 (and AVX2 disabled), the checksum changes to AB1CED180818F52B.

Regression?

Reproduces on: .NET 8.0 (Release Build)
Does NOT reproduce on: .NET 10.0 Preview (The behavior seems consistent on .NET 10, suggesting it might be fixed or behavior changed in main branch).

Known Workarounds

No response

Configuration

No response

Other information

The reproduction code includes:
A PGO warmup loop (40 iterations).
A ResetStatics method to ensure memory is clean between iterations.
Interfaces and Checksum logic required to run the generated code.

ReproTest.zip

[dotnet-policy-service]

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

[JulieLeeMSFT]

@AndyAyersMS PTAL.

[AndyAyersMS]

Thanks for the report. We'll investigate.

Since this JitStressRegs is not a supported mode for running the JIT, this may not end up being something we'll fix.

[AndyAyersMS]

Seems like the issue is in 8.0 with TC disabled, and AB1CED180818F52B is the correct result. If so, maybe this is something we'll want to fix.

@lhr1101 do you agree?

DOTNET_EnableAVX2=0
DOTNET_JitStressRegs=2
8.0
CHECKSUM:AB1CED180818F52B
9.0
CHECKSUM:AB1CED180818F52B
10.0
CHECKSUM:AB1CED180818F52B

DOTNET_TieredCompilation=0
8.0
CHECKSUM:49B5F8F053C8A9CC
9.0
CHECKSUM:AB1CED180818F52B
10.0
CHECKSUM:AB1CED180818F52B

DOTNET_JitMinOpts=1
DOTNET_TieredCompilation=0
8.0
CHECKSUM:AB1CED180818F52B
9.0
CHECKSUM:AB1CED180818F52B
10.0
CHECKSUM:AB1CED180818F52B

Also it looks like you're using Fuzzlyn ... can you share more about what you're doing?

[lhr1101]

I agree with your observation. If .NET 9.0, 10.0, and 8.0 (MinOpts) all consistently produce AB1CED180818F52B, then it is highly likely that AB1... is indeed the correct result.
Yes, I am using Fuzzlyn as the core code generator。
I automate the combination of TieredPGO, OSR, and various Stress modes (like JitStressRegs) to hunt for optimization regressions.

[AndyAyersMS]

This is a bug in loop hoisting on 8.0.

Specifically, if we have a code like:

 static int s_2;

 for (sbyte i = -126; i > -128; i--)           ;; outer loop L00
 {
     for (byte j = 2; j > 0; j--)              ;; inner loop L01
     {
         s_rt.Checksum("c_0", var4);           ;; virtual call writes memory
     }
     s_2 = 1;                                  ;;  outer-loop store
     s_2 = s_2--;                              ;;  outer-loop load / store
 }

The load in s_2-- has a known value (1) but is variant in loop L00. However, its memory dependence comes from the memory update in the inner loop L01. When considering the memory dependence in optRecordLoopMemoryDependence, the code checks if L00 is contained in L01, and since it isn't, the JIT doesn't record any memory dependence, and the load of s_2 is then hoisted from L00.

In 9.0 and later this computation was revised (see #106076) to check for dependence of each enclosing loop, so the load is noted as being dependent on L00 and hoisting is (properly) blocked.

The fix seems relatively surgical. I don't know the impact yet, looks like I would need to try and revive some part of SPMI for 8.0 or else use PMI.

@@ -7150,14 +7150,21 @@ void Compiler::optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, V
         updateLoopNum = updateParentLoopNum;
     }

-    // If the update block is not the header of a loop containing
-    // block, we can also ignore the update.
+    // If the memory definition is part of an ancestor loop of `loopNum` then
+    // `tree` depends on memory defined in that ancestor loop. Walk up the parent
+    // chain of `updateLoopNum` looking for such an ancestor. Otherwise ignore
+    // the update.
     //
-    if (!optLoopContains(updateLoopNum, loopNum))
+    while ((updateLoopNum != BasicBlock::NOT_IN_LOOP) && !optLoopContains(updateLoopNum, loopNum))
     {
-        JITDUMP("      ==> Not updating loop memory dependence of [%06u]/" FMT_LP ", memory " FMT_VN "/" FMT_LP
-                " is not defined in an enclosing loop\n",
-                dspTreeID(tree), loopNum, memoryVN, updateLoopNum);
+        updateLoopNum = optLoopTable[updateLoopNum].lpParent;
+    }
+
+    if (updateLoopNum == BasicBlock::NOT_IN_LOOP)
+    {
+        JITDUMP("      ==> Not updating loop memory dependence of [%06u]/" FMT_LP ", memory " FMT_VN
+                " is not dependent on an ancestor loop\n",
+                dspTreeID(tree), loopNum, memoryVN);
         return;
     }

fyi @jakobbotsch

Since the only reports of this are from Fuzzlyn I don't know if it will pass the servicing bar. I will work up and test a fix nonetheless.

[AndyAyersMS]

Ran a bespoke local SPMI collection for libraries and there were no diffs. #106076 also reported no diffs.

[AndyAyersMS]

This will be fixed in an upcoming .NET 8 release. @lhr1101 thanks for reporting it.

Any more luck finding issues with Fuzzlyn?