Skip to content

JIT: fix TrailingZeroCount constant folding on 32-bit targets - #133093

Open
AndyAyersMS wants to merge 4 commits into
dotnet:mainfrom
AndyAyersMS:fix-tzc-const-32bit
Open

JIT: fix TrailingZeroCount constant folding on 32-bit targets#133093
AndyAyersMS wants to merge 4 commits into
dotnet:mainfrom
AndyAyersMS:fix-tzc-const-32bit

Conversation

@AndyAyersMS

Copy link
Copy Markdown
Member

Fixes#133092.

BitOperations.TrailingZeroCount with a compile-time 64-bit constant produced a TYP_LONG importer node where the intrinsic returns int. On 32-bit targets that surfaced as InvalidProgramException at runtime, and as an importer assert in a checked JIT. It has been broken since #80789 (Jan 2023), so .NET 8, 9 and 10 are affected; .NET 6 and 7 are correct.

The fix

impPrimitiveNamedIntrinsic already reconciles a LONG/INT mismatch at the end of the function:

if ((result != nullptr) && (retType != baseType))
{
// We're either LONG->INT or INT->LONG
result = gtFoldExpr(gtNewCastNode(retType, result, /* unsigned */true, retType));
}

The baseType = retType; in the TrailingZeroCount constant path made that condition false, so the LONG->INT cast never ran. Removing it lets the existing reconciliation do its job.

This restores the shape the sibling intrinsics already have:

  • NI_PRIMITIVE_LeadingZeroCount builds the identical gtNewLconNode but has no baseType assignment, so its cast fires — and LZC is correct on x86 today.
  • NI_PRIMITIVE_Log2 assigns baseTypeafter inserting a cast, not instead of one.

On 64-bit targets the cast is now inserted where previously it was skipped; since result is a constant, gtFoldExpr folds it, so the emitted value is unchanged.

Verification

Measured on stock x86 runtimes before the fix — same net6.0 binary, DOTNET_TieredCompilation=0:

runtimeTZC(0x100000000UL)
x86 6.0.36, 7.0.2032 — correct
x86 8.0.30, 9.0.19, 10.0.11InvalidProgramException
x64, same binary32 — correct

A/B with a checked x86 altjit over a 1,200-method generated corpus:

altjitmodeexitmethods compiled
unfixedbase0xC0000003347 / 1200
unfixedJitStress=20xC0000003344 / 1200
fixedbase01200 / 1200
fixedJitStress=201200 / 1200

The folded constants were disassembled to confirm they are correct rather than merely assert-free: TZC(0)→mov eax, 64, TZC(1)→xor eax, eax, TZC(2)→1, TZC(1<<32)→32, TZC(1<<63)→63.

Note on the regression test

The malformed node was produced for every constant form, but only constants that Roslyn must encode as ldc.i8 escalate to a failure on a release JIT. Constants encoded as ldc.i4* + conv.i8 — such as 0UL, or 0xFFFFFFFFFFFFFFFF via ldc.i4.m1 — returned correct values even on the unfixed compiler, while still tripping the assert on a checked JIT.

The test therefore leads with 1<<32, 1<<63 and 0x100000001, which are the cases that actually fail without the fix. A test written only with 0UL would pass against the unfixed compiler in a release run. It also includes LeadingZeroCount, PopCount and a non-constant TrailingZeroCount as controls.

How this was found

A differential fuzzer that compiles generated methods with crossgen2 across four target architectures. The bug was reachable for ~3 years but invisible to the existing campaign because every machine in it is x64 or arm64; compiling for 32-bit targets does not require 32-bit hardware.

The importer's constant-folding path for `BitOperations.TrailingZeroCount`
builds a `TYP_LONG` node for a 64-bit operand:
result = gtNewLconNode(BitOperations::TrailingZeroCount(cns));
but `TrailingZeroCount` returns `int`. `impPrimitiveNamedIntrinsic` already
reconciles that at the end of the function:
if ((result != nullptr) && (retType != baseType))
{
// We're either LONG->INT or INT->LONG
result = gtFoldExpr(gtNewCastNode(retType, result, true, retType));
}
The `baseType = retType;` in the constant path made that condition false, so
the LONG->INT cast never ran and a `TYP_LONG` value reached a context expecting
`int`. On 32-bit targets that surfaced as `InvalidProgramException` at runtime
and as an importer assert in a checked JIT; on 64-bit targets the malformed
node happened to be harmless.
`NI_PRIMITIVE_LeadingZeroCount` has the identical constant path but no
`baseType` assignment, so its cast fires and it is correct on x86 --
which is the shape this restores. `NI_PRIMITIVE_Log2` assigns `baseType`
*after* inserting a cast, not instead of one.
Fixesdotnet#133092
CopilotAI lite review requested due to automatic review settings September 2, 2026 15:08
@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 2, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new regression test directory is missing a matching Runtime_133092.csproj, so the test likely won’t be built/executed in CI (and may miss required env settings like DOTNET_TieredCompilation=0).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a CoreCLR JIT importer type-mismatch in the BitOperations.TrailingZeroCount(ulong) constant-folding path so 32-bit targets don’t end up with a TYP_LONG node where an int is required, and adds a regression test intended to cover the failing constant forms.

Changes:

  • Remove the baseType = retType; assignment in the NI_PRIMITIVE_TrailingZeroCount constant path so the existing LONG↔INT reconciliation logic runs.
  • Add a new JitBlue regression test that exercises TrailingZeroCount constant folding (including ldc.i8-encoded constants) plus a few control cases.
File summaries
FileDescription
src/coreclr/jit/importercalls.cppEnsures TrailingZeroCount constant folding preserves the operand type so the end-of-function cast reconciliation can narrow longint when needed.
src/tests/JIT/Regression/JitBlue/Runtime_133092/Runtime_133092.csAdds a regression test for constant-folded TrailingZeroCount(ulong) cases that previously failed on 32-bit targets.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Local A/B on this exact commit, using a checked x86 altjit and crossgen2 built from this branch (so the JIT-EE version matches and there is no cross-tree skew).

With the fix:

crossgen2 --targetarch:x86 -r <x86 10.0.11 framework>/*.dll -o tzc_x86.r2r.dll tzc.dll
Emitting R2R PE file: tzc_x86.r2r.dll
exit=0

Negative control — same tree, same crossgen2, only baseType = retType; restored:

importer.cpp:11597
Assertion failed '... Possibly bad IL with CEE_ret at offset 000Eh (op1=NULL op2=long stkDepth=0)'
in 'P:Const_ldc_i8_1shl32():int' during 'Importation'
in 'P:Const_ldc_i8_1shl63():int'
in 'P:Const_ldc_i8_tzc0():int'
in 'P:Const_ldc_i4_allones():int'
exit=-2147483645

Only the TrailingZeroCount constant methods assert. The Var(v) (non-constant), LeadingZeroCount(const) and PopCount(const) methods in the same assembly compile cleanly in both runs, so the difference is attributable to this one line rather than to the harness or the toolchain.

Worth noting for anyone reading the issue's trigger discussion: Const_ldc_i4_allonesTrailingZeroCount(0xFFFFFFFFFFFFFFFFUL), which Roslyn encodes as ldc.i4.m1 + conv.i8does assert here. That confirms the malformed node is produced for every constant form on a checked JIT; the ldc.i8 distinction only governs whether it escalates to InvalidProgramException on a release JIT. The regression test covers both groups for that reason.

Build check: clr.jit, clr.alljits (which includes clrjit_win_x86_x64 and clrjit_universal_arm_x64) and clr.tools all build with 0 warnings and 0 errors, Release and Checked.

Comment threadsrc/coreclr/jit/importercalls.cpp

@tannergoodingtannergooding left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Had to think through the handling because its a bit nuanced.

Might still be a valuenum issue, but I've not fully thought through whether or not that is an issue or not.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1c0b5362-6ffc-4f6d-8bdf-7adf4c55ca61
CopilotAI review requested due to automatic review settings September 2, 2026 22:18

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is minimal, aligns with existing importer reconciliation behavior, and is covered by an appropriate regression test added to the JIT test suite.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@tannergooding can you re-approve? Had to fix test integration.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@copilot resolve the merge conflicts in this pull request

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b83a30c2-8c19-451e-aca1-c471bdd3cb2f
CopilotAI review requested due to automatic review settings September 4, 2026 16:12

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix is minimal, consistent with adjacent intrinsic patterns, and is covered by a targeted regression test.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@tannergooding

Copy link
Copy Markdown
Member

Looks like there's a merge conflict again....

I wonder if we can reorganize the Regression_ro_*.csproj merged test runner projects to auto include the compilation files so we can avoid this.

CC. @jkoritzinsky as I believe you were looking at some of the merged test runner stuff recently

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b83a30c2-8c19-451e-aca1-c471bdd3cb2f
CopilotAI review requested due to automatic review settings September 4, 2026 19:01

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is narrowly scoped, aligns the TZC constant-folding path with existing sibling intrinsic patterns, and includes a targeted regression test to prevent recurrence.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: BitOperations.TrailingZeroCount(<64-bit constant>) produces InvalidProgramException on 32-bit targets

3 participants

@AndyAyersMS@tannergooding