Skip to content
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

Fix BOX import to not create "wrong" casts #57094

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

SingleAccretion
Copy link
Contributor

All casts in the IR have TYP_INT, except for this one case. Fix it by using genActualType.

Also add a comment explaining the reason a cast is needed at all in case the types are small.

No diffs as per SPMI. Note: it looks like the Enum.HasFlag optimization has been fixed to use actual types.

Fixes #10843

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 9, 2021
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 9, 2021
All casts in the IR have TYP_INT, except for this one
case. Fix it by using genActualType.

Also add a comment explaining the reason a cast is needed
at all in case the types are small.
Comment on lines -6865 to -6866
assert((varTypeIsFloating(srcTyp) && varTypeIsFloating(dstTyp)) ||
(varTypeIsIntegral(srcTyp) && varTypeIsIntegral(dstTyp)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert was redundant with the one above the if.

@SingleAccretion SingleAccretion marked this pull request as ready for review August 10, 2021 08:16
@sandreenko sandreenko added this to the 7.0.0 milestone Aug 10, 2021
@hughbe
Copy link
Contributor

hughbe commented Aug 10, 2021

It would be good to have a test with this change

@SingleAccretion
Copy link
Contributor Author

What kind do you have in mind? This change is not fixing a bug per se, just an inconsistency internal to the IR.

@hughbe
Copy link
Contributor

hughbe commented Aug 10, 2021

I was thinking having tests that make sure HasFlag still produces correct results for Enums of all different sizes (unsigned/signed, 8/16/32/64 bit), e.g.

enum ShortEnum : short
{
    None,
    Boo
}

static bool Test(ShortEnum s) => s.HasFlag(ShortEnum.Boo);

@SingleAccretion
Copy link
Contributor Author

I see. We already have a test for the optimization here, though it deals more with the intricacies of how many flavors of boxes can one see. I will see about adding some more coverage of small type mixes, though I do not expect us to have issues in this area specifically (there are definitely issues with dealing with these implicit truncations in general...).

@AndyAyersMS
Copy link
Member

@JulieLeeMSFT you can cross this one off the list I gave you the other day.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM.

@AndyAyersMS AndyAyersMS merged commit e74bc2a into dotnet:main Aug 25, 2021
@SingleAccretion SingleAccretion deleted the Fix-BOX-Import branch August 26, 2021 00:20
@SingleAccretion SingleAccretion restored the Fix-BOX-Import branch August 31, 2021 16:04
@SingleAccretion SingleAccretion deleted the Fix-BOX-Import branch August 31, 2021 16:08
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix type of cast node generated by box import
4 participants