-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[clr-interp] Fix implicit promotion of I4 to I #122319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clr-interp] Fix implicit promotion of I4 to I #122319
Conversation
…allow for more useful optimization tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes implicit promotion of I4 (int32) to I (native int) in the CoreCLR interpreter to match RyuJIT's behavior. The fix addresses inconsistencies where some opcodes use zero-extension and others use sign-extension when promoting 32-bit integers to native integers on 64-bit platforms.
- Adds a new helper function to determine the correct widening operation (sign or zero extension) based on the opcode
- Implements comprehensive tests for all opcodes that perform implicit promotion, covering arithmetic, comparison, and branch operations
- Tests verify correct behavior for unsigned overflow operations (add.ovf.un, sub.ovf.un, mul.ovf.un) and unsigned branch/comparison operations (bne.un, blt.un, etc.)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/interpreter/compiler.cpp | Adds InterpOpForWideningArgForImplicitUpcast() to select appropriate conversion (sign vs zero extension) and updates EmitTwoArgBranch() and EmitBinaryArithmeticOp() to use it |
| src/tests/JIT/Methodical/int64/unsigned/implicit_promotion_il.il | New IL assembly defining 52 operator methods testing all promotion scenarios with mixed I4/I8 operands |
| src/tests/JIT/Methodical/int64/unsigned/implicit_promotion_il.ilproj | Project file for the IL test |
| src/tests/JIT/Methodical/int64/unsigned/implicit_promotion.cs | New C# test invoking IL operators with negative values to validate sign/zero extension behavior on 32-bit vs 64-bit platforms |
| src/tests/JIT/Methodical/Methodical_*.csproj | Updates to include the new test files in various build configurations |
src/tests/JIT/Methodical/int64/unsigned/implicit_promotion_il.il
Outdated
Show resolved
Hide resolved
src/tests/JIT/Methodical/int64/unsigned/implicit_promotion_il.il
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
janvorli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the copilot nits that seem to make sense.
Co-authored-by: Copilot <[email protected]>
Ryujit does implicit promotion using zero-extending promotions for some opcode and sign extending for others. I don't see a particularly good rhyme or reason for the current behavior, but it does not match what the interpreter does.
This PR adds a test for all the opcodes that experience promotion, and attempts to test all the behaviors correctly. In building it, several issues were found/fixed.
This is known to fix the GitHub13501 test. in addition to the various issues found during test development.