JIT: fix FitsIn<int32_t> assert in BitOperations.Rotate{Left,Right} const-fold#129136
JIT: fix FitsIn<int32_t> assert in BitOperations.Rotate{Left,Right} const-fold#129136AndyAyersMS wants to merge 4 commits into
Conversation
…onst-fold NI_PRIMITIVE_RotateLeft/Right's constant-folding path passes the unsigned fold result directly to gtNewIconNode(ssize_t, TYP_INT). For TYP_INT/TYP_UINT operands with the high bit set (e.g. RotateRight(0xFFFFFFFFu, k) which folds to 0xFFFFFFFF), the implicit uint32_t-to-ssize_t conversion zero-extends to a positive value (4294967295) that does not fit in int32_t, tripping GenTreeIntCon::SetIconValue's FitsIn<int32_t>(value) assert during 'Morph - Global'. Cast through int32_t first so the sign bit is preserved when widened to ssize_t. Fixes dotnet#129099. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull request overview
Fixes a JIT constant-folding bug for NI_PRIMITIVE_RotateLeft / NI_PRIMITIVE_RotateRight where 32-bit rotate results with the high bit set could be represented as a zero-extended ssize_t, ultimately tripping int32-range assertions during later IR mutation.
Changes:
- Adjust rotate constant-folding to cast the folded 32-bit result through
int32_tbefore creating theGT_CNS_INTnode. - Add a new JitBlue regression test covering
RotateLeft/RotateRightfolding for high-bit-set inputs and wire it into the regression project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/coreclr/jit/importercalls.cpp | Casts 32-bit rotate fold results through int32_t to preserve sign when widened and avoid later int32-range asserts. |
| src/tests/JIT/Regression/JitBlue/Runtime_129099/Runtime_129099.cs | Adds a regression test that forces materialization of folded rotate results (via volatile sinks) and validates expected outputs. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new Runtime_129099 regression test in the JIT regression build. |
| // Sign-extend the unsigned fold result to int32_t so gtNewIconNode's | ||
| // FitsIn<int32_t>(value) check for TYP_INT/TYP_UINT folds doesn't trip | ||
| // on the high-bit-set case (e.g. RotateLeft(0xFFFFFFFFu, k)). | ||
| result = gtNewIconNode( | ||
| static_cast<int32_t>(BitOperations::RotateLeft(cns1, cns2)), baseType); |
| // Sign-extend the unsigned fold result to int32_t so gtNewIconNode's | ||
| // FitsIn<int32_t>(value) check for TYP_INT/TYP_UINT folds doesn't trip | ||
| // on the high-bit-set case (e.g. RotateRight(0xFFFFFFFFu, k)). | ||
| result = gtNewIconNode( | ||
| static_cast<int32_t>(BitOperations::RotateRight(cns1, cns2)), baseType); |
| // NI_PRIMITIVE_RotateLeft/Right's const-fold path stored the unsigned | ||
| // fold result into gtNewIconNode(ssize_t, TYP_INT). For uint operands | ||
| // with the high bit set (e.g. RotateRight(0xFFFFFFFFu, k)) the result |
| result = gtNewIconNode( | ||
| static_cast<int32_t>(BitOperations::RotateLeft(cns1, cns2)), baseType); |
There was a problem hiding this comment.
Is this the best/correct way to do it?
That is, this seems like a general issue with gtNewIconNode(ssize_t) since we default to TYP_INT and so that scenario should really have an assert(FitsIn<int32_t>(ssize_t)) or insert the static_cast<int32_t>(value) itself, since anything else is just "incorrect IR"
Anything that doesn't fit rather should be TYP_LONG and should've gone through gtNewLconNode instead (or possibly TYP_BYREF on 64-bit for the few cases that have it).
I wonder if even the general signature of gtNewIconNode is "incorrect" and if it rather should be int32_t instead, to help enforce correctness here; particularly since any larger value may need to be LconNode to work on 32-bit.
There was a problem hiding this comment.
Note
Reply is AI-generated (GitHub Copilot CLI).
Agreed — the call-site cast is a workaround for a missing API contract. I've pushed an update that also adds the invariant at gtNewIconNode itself:
GenTreeIntCon* Compiler::gtNewIconNode(ssize_t value, var_types type)
{
assert(genActualType(type) == type);
assert(genTypeSize(type) > genTypeSize(TYP_INT) || FitsIn<int32_t>(value));
return new (this, GT_CNS_INT) GenTreeIntCon(type, value);
}This catches the bug class at construction time rather than per-call-site. Smoke-verified locally:
- All built
Regression_ro_*JIT regression tests still pass - A 120-trial ReifyCs sweep produces no firings of the new assert
- The fix-129099 regression test continues to fail without the call-site fix (the assert at
gtNewIconNodewould also fire) and pass with it
On the deeper signature question (int32_t vs ssize_t): a much larger refactor that touches 455 call sites — many of which legitimately want ssize_t for TYP_I_IMPL/TYP_BYREF/TYP_LONG use. The assert above gives us the safety net without the API churn. I'd suggest filing a separate issue for the signature change if you want to pursue it.
There was a problem hiding this comment.
Good question. I don't know if int32_t is the right direction but will dig into it some.
There's something unnecessarily clunky about the icon nodes in general. Not sure I want to revisit that right now though.
There was a problem hiding this comment.
Interesting... AI is getting ahead of itself here and just replying on its own. Let me reign it in a bit.
There was a problem hiding this comment.
Yeah, a bit unsure as to the direction myself, but I think assert is a good starting point and will help catch any other issues longer term.
- Comments now correctly attribute the FitsIn<int32_t> assert to the downstream SetIconValue/BashToConst call (the construction itself is unchecked). Updated both the call-site comments and the regression test's header. - Add the same FitsIn<int32_t> assert to gtNewIconNode itself (per @tannergooding) so this invariant is enforced at the IR-construction boundary, not just at the rotate call site. Anything larger than int32 should go through gtNewLconNode (TYP_LONG) or use TYP_I_IMPL on 64-bit. Verified: all built Regression_ro_*.dll tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Looks like this has turned up at least 4 other places needing a fix. Let me think about the right way to proceed here. |
|
Note This comment was authored by GitHub Copilot CLI on @AndyAyersMS's machine. Looking at the 27 CI failures on this PR's last build (1455021), the new
So really two distinct JIT sites, both the same pattern: a Candidate patch--- a/src/coreclr/jit/gentree.cpp
+++ b/src/coreclr/jit/gentree.cpp
@@ -25438,7 +25438,7 @@ GenTree* Compiler::gtNewSimdIsNegativeInfinityNode(var_types type,
else if (simdBaseType == TYP_FLOAT)
{
simdBaseType = TYP_UINT;
- cnsNode = gtNewIconNode(0xFF800000);
+ cnsNode = gtNewIconNode(static_cast<int32_t>(0xFF800000));
}
--- a/src/coreclr/jit/lowerxarch.cpp
+++ b/src/coreclr/jit/lowerxarch.cpp
@@ -812,7 +812,7 @@ void Lowering::LowerCastFloatToInt(...)
convertIntrinsic = TargetArchitecture::Is64Bit
? NI_X86Base_X64_ConvertToInt64WithTruncation
: NI_X86Base_ConvertToVector128Int32WithTruncation;
- maxIntegralValue = m_compiler->gtNewIconNode(static_cast<ssize_t>(UINT32_MAX));
+ maxIntegralValue = m_compiler->gtNewIconNode(static_cast<int32_t>(UINT32_MAX));
minFloatOverflow = 4294967296.0; // 2^32;
break;
}VerificationApplied this PR's two-hunk patch plus the candidate fix locally on Fuzzer cross-checkJust as a sanity input — last night's ReifyCs run produced 31 unique crash seeds across 5 fertile profiles (intrinsic / struct_intrinsic / mixed_sign / mixed_types / stage2_eh) all hitting the original Happy to push the candidate patches to this PR (or to a separate one) if the direction is right — let me know. |
These are the additional sites that trip the new assert added to
gtNewIconNode in the previous commit. Same root cause and same fix
shape -- sign-extend a uint bit pattern through int32_t so the
ssize_t parameter fits in int32_t for a TYP_INT-sized const.
* gentree.cpp gtNewSimdIsNegativeInfinityNode: 0xFF800000 (the
single negative-infinity bit pattern) used as a TYP_UINT broadcast.
* lowerxarch.cpp LowerCastFloatToInt: UINT32_MAX used as the
saturation bound when lowering double-to-uint.
Surfaced via the prior CI build (1455021) which hit these on
System.Numerics.Vector{2,3,4}.IsNegativeInfinity (Importation) and
System.Convert.ToUInt32(double) / NumericConvertInstruction.ConvertDouble
(Lowering nodeinfo).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed
Built locally on windows.x64.Checked. The smoke that crashed both asserts before now returns 100. Note I noticed a local-only |
| ? NI_X86Base_X64_ConvertToInt64WithTruncation | ||
| : NI_X86Base_ConvertToVector128Int32WithTruncation; | ||
| maxIntegralValue = m_compiler->gtNewIconNode(static_cast<ssize_t>(UINT32_MAX)); | ||
| maxIntegralValue = m_compiler->gtNewIconNode(static_cast<int32_t>(UINT32_MAX)); |
| // For TYP_INT-sized constants the value must fit in int32_t; otherwise the | ||
| // node is in an invalid state and downstream SetIconValue / BashToConst | ||
| // will assert when the constant is updated. Wider values should use | ||
| // gtNewLconNode (TYP_LONG) or TYP_I_IMPL on 64-bit targets. | ||
| assert(genTypeSize(type) > genTypeSize(TYP_INT) || FitsIn<int32_t>(value)); |
|
Resumed fuzzing with both pushed fixes baked into a runtime-a build (commit Instrumented Source line that triggers it: That's exactly the 3rd site your local If you'd like, I can cherry-pick your Fuzz is continuing through P1B–E + Phase 2 — will report any new signatures as they surface. |
Third site for the same bug class. Hits any C# expression of the form
`x %u (uint)C` where C has bit 31 set: divisorValue is computed as
a ssize_t after masking to UINT32_MAX and passed to gtNewIconNode
with TYP_INT, which trips the assert added earlier in this PR.
Sign-extend through int32_t so the constant round-trips correctly
(same shape as the importercalls.cpp Rotate{Left,Right} fix in the
first commit and the IsNegativeInfinity / UINT32_MAX fixes in the
prior commit).
Independently surfaced 31 times by a post-fix fuzz run across 5
profiles -- now the dominant signature for this assert class once
the const-fold and prior gtNewIconNode sites are fixed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Post-fix fuzz complete — 37,000 trials, 31 crashes, all the same UMOD signature. Pushed Run setup
Phase 1 — re-run the 5 originally fertile #129099 profiles
The original Phase 2 — exercise the JIT areas my new fixes touched
No new bug shapes. The 1 mismatch is the known narrow-int interpreter false positive. Fix applied (
|
Note
PR description is AI-generated (GitHub Copilot CLI). The investigation, fix, and regression test are checked by me.
Fixes #129099.
Root cause
NI_PRIMITIVE_RotateLeft/NI_PRIMITIVE_RotateRight's constant-folding path (importercalls.cpp) passes the unsigned fold result directly togtNewIconNode(ssize_t, TYP_INT):For
TYP_INT/TYP_UINToperands with the high bit set — e.g.BitOperations.RotateRight(0xFFFFFFFFu, k)which folds to0xFFFFFFFF— the implicituint32_t→ssize_tconversion zero-extends to a positive value (4294967295) that does not fit inint32_t.GenTreeIntCon::SetIconValuethen trips itsFitsIn<int32_t>(value)assert duringMorph - Global.The
ulongoverload is unaffected because it usesgtNewLconNode(64-bit).Fix
Cast through
int32_tfirst so the sign bit is preserved when widened tossize_t:result = gtNewIconNode( static_cast<int32_t>(BitOperations::RotateLeft(cns1, cns2)), baseType);RotateLeft(0xFFFFFFFFu, 1)is still0xFFFFFFFFu; reinterpreting that asint32_tgives-1; widening-1tossize_tgives-1;FitsIn<int32_t>(-1)istrue. Downstream consumers reading the IconValue as auint32_t(viastatic_cast<uint32_t>(IconValue)) recover0xFFFFFFFFcorrectly.Regression test
src/tests/JIT/Regression/JitBlue/Runtime_129099/Runtime_129099.csexercises bothRotateLeftandRotateRighton0xFFFFFFFFu,0x80000000u(high-bit-only), andint.RotateRight(-1, _)(signed-int overload, sanity). Each call stores into avolatilestatic so the fold result must be materialized — without this the JIT can dead-code-eliminate the call before the assert fires.Wired into
src/tests/JIT/Regression/Regression_ro_2.csproj.Verified locally on
osx-arm64.Checked:Assert failure ... 'FitsIn<int32_t>(value)' ... 'Runtime_129099:FoldRotateRightUInt():uint' during 'Morph - Global'A ~3,000-trial ReifyCs sweep that previously found 7 instances of this assert now finds 0.