Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Jun 23, 2025

  • Implements the _un versions of conv_ovf.
  • Fixes some incorrect/missing conversions in the original conv_ovfs I added in a previous PR.
  • Expands the conv_ovf helper so it can do same-size and growing conversions with sign changes.

@Copilot Copilot AI review requested due to automatic review settings June 23, 2025 18:10
@kg kg requested review from BrzVlad and janvorli as code owners June 23, 2025 18:10
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 23, 2025
Copilot

This comment was marked as outdated.

@kg kg marked this pull request as draft June 23, 2025 18:27
@kg
Copy link
Member Author

kg commented Jun 23, 2025

Noticed some other problems with our conversions so I'm going to clean them up.

@kg kg requested a review from Copilot June 23, 2025 20:09
Copy link
Contributor

@Copilot Copilot AI left a 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 adds support for unsigned-source overflow-checking conversions in the interpreter backend and corresponding compiler support, and adds a new unit test to validate these conversions.

  • Introduces new conv.u8.u4 and various conv.ovf.* opcodes in intops.def
  • Extends ConvOvfHelper and opcode dispatch in interpexec.cpp to handle unsigned-source cases
  • Updates compiler.cpp to emit the new overflow opcodes for unsigned conversions
  • Adds TestConvOvfUn (and console logging) in Interpreter.cs to cover unsigned overflow conversions

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/tests/JIT/interpreter/Interpreter.cs Added TestConvOvfUn method and Console.WriteLine calls for clarity
src/coreclr/vm/interpexec.cpp Enhanced ConvOvfHelper to support growing conversions with sign changes and added unsigned conversion cases
src/coreclr/interpreter/intops.def Defined new unsigned-source overflow-checking opcodes
src/coreclr/interpreter/compiler.cpp Updated code generator to emit the new unsigned conversion opcodes
Comments suppressed due to low confidence (2)

src/coreclr/interpreter/compiler.cpp:4078

  • In the CEE_CONV_OVF_U_UN case on 32-bit targets, using INTOP_CONV_OVF_U4_I8 treats the source as signed. To properly implement 'un' (treat source as unsigned), replace this with INTOP_CONV_OVF_U4_U8.
#else

src/tests/JIT/interpreter/Interpreter.cs:1570

  • [nitpick] Variables a, b, and c are non-descriptive; consider renaming them to clarify their purpose (e.g., resultFromU2, resultFromU4, resultFromU8).
            byte a = (byte)u2,

@teo-tsirpanis teo-tsirpanis added area-CodeGen-Interpreter-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 23, 2025
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

@kg kg marked this pull request as ready for review June 23, 2025 22:08
@janvorli
Copy link
Member

janvorli commented Jul 7, 2025

@kg can you please resolve the conflict so that we can merge the change?

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Checkpoint

Checkpoint

Implement most of the unsigned source conversions

Generalize ConvOvfHelper to support growing conversions as well
Add some missing conv ovf opcodes
Fix up ovf conversion mappings in compiler

Fix copy paste error

Fix copy-paste error

Add missing check

Remove no-op movs

Remove no-op mov convs
@kg
Copy link
Member Author

kg commented Jul 8, 2025

I see "Merging is blocked: All comments must be resolved" but there are no unresolved comments on this PR. Does anyone know what causes that?

EDIT: Copilot had marked some unresolved comments as "outdated", making them invisible.

@kg kg merged commit 8ec637d into dotnet:main Jul 8, 2025
101 of 103 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants