-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[mono][interp] Sign extend I4 return value since jit-ed code expects it #117306
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
Conversation
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 ensures that 32-bit integer return values from interpreted methods on amd64 are sign-extended to match JIT expectations and updates the call-result widening logic to include the interpreter backend alongside LLVM.
- Sign-extend I4 return values in the amd64 interpreter call context.
- Extend
mono_emit_widen_call_res
to treat the interpreter path similarly to LLVM for return-value widening.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/mono/mono/mini/mini-amd64.c | Sign-extend I4 return value on the amd64 interpreter path when returning in RAX. |
src/mono/mono/mini/method-to-ir.c | Rename and extend the “might_use_llvm” flag to also cover the interpreter path. |
Comments suppressed due to low confidence (2)
src/mono/mono/mini/method-to-ir.c:2238
- Update this comment to mention that widening now applies for both LLVM and the interpreter backends, not just LLVM.
// LLVM code doesn't uses zero extend the full word while jit expects it.
src/mono/mono/mini/mini-amd64.c:1480
- Add a unit test for the interpreter on amd64 to verify that negative I4 return values are correctly sign-extended when placed in RAX.
if (sig->ret->type == MONO_TYPE_I4 && ainfo->storage == ArgInIReg && ainfo->reg == AMD64_RAX)
Tagging subscribers to this area: @steveisok, @vitek-karas |
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!
Replaced by #117336 |
Fixes #115859