Fix host-dependent assertion prop non-null proof#128510
Conversation
Avoid treating an empty short assertion bitset as uninitialized during global assertion propagation. Global AP can still prove PHI values non-null by walking predecessor edge assertions, so the MayBeUninit guard should only short-circuit local assertion propagation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adjusts Compiler::optAssertionIsNonNull to avoid bailing out on BitVecOps::MayBeUninit(assertions) during global assertion propagation, so global VN/PHI predecessor-edge assertion walking can still prove values non-null even when the current block’s assertion set is empty.
Changes:
- Restricts the
MayBeUninit(assertions)early-return to local assertion propagation (optLocalAssertionProp). - Leaves global propagation free to continue into VN-based non-null proof paths even with an “empty” assertion set representation.
| } | ||
|
|
||
| if (!optCanPropNonNull || BitVecOps::MayBeUninit(assertions)) | ||
| if (!optCanPropNonNull || (optLocalAssertionProp && BitVecOps::MayBeUninit(assertions))) |
|
TBH, the fix doesn't make any sense to me (and as you can see, it is failing on CI), what exactly are you trying to fix, is there a repro? |
|
It looks like |
The failure is in this pipeline: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1430598&view=logs&jobId=28c8af7e-f827-5a8a-84e8-48ab1f161753&j=28c8af7e-f827-5a8a-84e8-48ab1f161753&t=f6b2d862-4a75-5fb7-0976-72cd932a52b6 The x64-host crossbuild targeting arm differs from the arm-native build. arm-native Microsoft.Extensions.Logging.Console: https://helixr18s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-128235-merge-f23053d080cb46bdaa/WorkItem/1/Microsoft.Extensions.Logging.Console.ni.dll?helixlogtype=result x64-host crossbuilt: Microsoft.Extensions.Logging.Console.ni.x64.zip from the larger zip here The method |
Two host-independent leaks of an uninitialized ASSERT_TP were causing crossgen2 output to differ between 64-bit and 32-bit hosts (and were hidden by an overly aggressive MayBeUninit short-circuit in optAssertionIsNonNull): 1. optVnNonNullPropCurStmt creates 'ASSERT_TP empty = BitVecOps::UninitVal()' and passes it through optAssertionIsNonNull into optAssertionVNIsNonNull. In short-rep an empty bitset is indistinguishable from uninit, so the bail-out incidentally worked on 64-bit hosts; on 32-bit hosts the bitset was long-rep and the same input was treated as non-empty, producing different codegen (and an assertion fire once the bail-out was loosened). Substitute MakeEmpty for uninit at the global-AP call site so the iter walks a valid empty bitset. 2. optGetEdgeAssertions reads bbAssertionOut / bbJtrueAssertionOut[bbNum] for predecessor blocks. Blocks added after optInitAssertionDataflowFlags have no valid entry (bbJtrueAssertionOut is sized to fgBBNumMax+1 at init time, and bbAssertionOut shares storage with bbCseOut so its contents are stale rather than nullptr). Capture optDataflowBBNumMax at init time and treat any predecessor whose bbNum exceeds it as carrying no assertions. With both fixes, all 257 framework assemblies produce identical R2R output when crossgen2 runs on linux-x64 vs linux-arm targeting linux-arm, including the 13 assemblies that previously diverged. See dotnet#128602. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Filed #128602. As a reference for investigation I pushed copilot's works to get this to compile, but it feels hacky. I'm not familiar enough to know the right solution here, so I'll just disable these assemblies for comparison tests for now. |
Crossgen2 was emitting different code for the same methods with null-coalescing statements when targeting ARM depending on which host it was on. On ARM (and I believe x86), it is able to prove the local is non-null, but on x64 it is not. My understanding (based on Copilot's investigation), is that the
MayBeUninit()check causes an early return on 64-bit systems, but not on 32-bit when the assertions bitmap is between 32 and 64 bits long. But whenoptLocalAssertionProp == falsewe may still return true fromoptAssertionVNIsNonNull(vn, assertions). So we want to make sureoptLocalAssertionPropis alsotruebefore we return early.Note
This PR description was generated by GitHub Copilot.
Summary
Restrict the
BitVecOps::MayBeUninit(assertions)bailout inoptAssertionIsNonNullto local assertion propagation. Global assertion propagation can still prove a value non-null by walking VN/PHI predecessor edge assertions, even when the current block's assertion set is empty.This fixes a host-dependent JIT behavior exposed by crossgen2 comparison runs: on x64, an empty short
BitSetShortLongRepisnullptrand aliasesUninitVal(), while on 32-bit ARM the same assertion table is long/heap-backed and empty is non-null. The old global-AP bailout therefore prevented x64-hosted ARM cross-JIT from proving a PHI non-null, but allowed native ARM-hosted JIT to do so.Validation
./build.sh clr.jit+clr.aot -c DebugMicrosoft.Extensions.Logging.Console.ConsoleLogger:LogRecordstargetinglinux-arm; output hash was80198ba3b1ae56900de7bc8de9d221e0a6be42b9c61ed2b951a35b3401e746dd, matching the native ARM-hosted behavior, and the disassembly no longer has the extra explicit null-check beforeStringWriter.GetStringBuilder.