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 |
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.