JIT: Allow optNarrowTree CAST case to match ULONG and LONG equivalently#129206
Conversation
|
@dhartglassMSFT PTAL Not many diffs, and again clustered in tests, but not a very big change either. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR JIT narrowing logic so optNarrowTree’s GT_CAST handling treats LONG and ULONG as equivalent (same actual type/width), enabling downstream ARM64 instruction combining to produce shifted-register forms in cases that previously got blocked by signedness mismatches in cast chains.
Changes:
- CoreCLR JIT: relax
optNarrowTree’sGT_CASTgate from exact-type equality togenActualType(...)equality, and broaden the “cast long” match to includeULONGviagenActualType(tree) == TYP_LONG. - ARM64 codegen validation: update existing Neg/CMN instruction-combining tests to expect single-instruction shifted-register forms (
neg ..., LSR #imm,cmn ..., LSR #imm). - Add/adjust tests to cover an MVN cast-chain LSR case and assert elimination of a redundant ARM64
movfor(uint)(ulong)x.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/optimizer.cpp | Allows optNarrowTree cast narrowing to proceed when srct and CastToType() differ only by signedness (e.g., LONG vs ULONG). |
| src/tests/JIT/opt/InstructionCombining/Neg.cs | Updates ARM64 disasm expectations for NegLSR to the shifted-register neg form. |
| src/tests/JIT/opt/InstructionCombining/Cmn.cs | Updates ARM64 disasm expectations for CmnLSR to the shifted-register cmn form. |
| src/tests/JIT/opt/InstructionCombining/Mvn.cs | Adds MvnCastChainLSR test and runtime check validating MVN+LSR combining through a cast chain. |
| src/tests/JIT/opt/InstructionCombining/Casts.cs | Tightens ARM64 expectation for (uint)(ulong)x to ensure no redundant mov remains. |
tannergooding
left a comment
There was a problem hiding this comment.
Changes LGTM, just a question about whether one change is actually doing anything and if we can maybe improve the comment.
|
going to bounce to trigger new CI |
|
Interestingly this has minopts diffs, evidently |
I think we have a few cases where opts like this aren't fully prevented in MinOpts, particularly via morph. We were discussing it on Discord a few weeks back and it'd probably be good to cleanup longer term, but obviously not critical to do right now. |
|
/ba-g unrelated test leg timing out |
Fixes #111888