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

[ARM64] - IsValidCompareChain should return false if both operands are not integral types #74853

Merged
merged 4 commits into from
Sep 2, 2022

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Aug 31, 2022

Description
Should resolve this assert: #74172

The assert was this in genCodeForConditionalCompare:

    // No float support or swapping op1 and op2 to generate cmp reg, imm.
    assert(!varTypeIsFloating(op2Type));

Seems like float types are not supported on conditional compares, ccmp.

The PR, #71705, introduced compare chains.
Compare chains will generate conditional compares, but the checks to see if an op is a valid compare chain did not include any type checks. This PR adds the checks; if the operands are integral types, op is a valid compare chain.

Acceptance Criteria

  • CI Passes

@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 31, 2022
@ghost ghost assigned TIHan Aug 31, 2022
@ghost
Copy link

ghost commented Aug 31, 2022

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

Issue Details

** Description **
Should resolve this assert: #74172

The assert was this in genCodeForConditionalCompare:

    // No float support or swapping op1 and op2 to generate cmp reg, imm.
    assert(!varTypeIsFloating(op2Type));

Seems like float types are not supported on conditional compares, ccmp.

Compare chains will generate conditional compares, but the checks to see if an op is a valid compare chain did not include any type checks. This PR adds the checks; if the operands are integral types, op is a valid compare chain.

** Acceptance Criteria **

  • CI Passes
  • Run JITStress CI to see the test case succeed
Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@TIHan
Copy link
Contributor Author

TIHan commented Aug 31, 2022

@kunalspathak @BruceForstall This is ready.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

ContainCheckCompareChain needs this check too. IsValidCompareChain is only used for the AND path because it wants to ensure both children can be converted before converting either of them.

@TIHan
Copy link
Contributor Author

TIHan commented Aug 31, 2022

Yea, looks like we need to do that as well. Though, we do not have a test case that is failing as a result of not adding those checks there.

@jakobbotsch
Copy link
Member

Indeed, that code won't be exercised until after #73472 is merged.

@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Sep 1, 2022
@a74nh
Copy link
Contributor

a74nh commented Sep 2, 2022

Change looks good to me.

Thanks for fixing this! (and apologies for introducing it :) )

@TIHan
Copy link
Contributor Author

TIHan commented Sep 2, 2022

@a74nh No need to apologize, not a problem at all! I mentioned that PR so we a history trail. I mean, @jakobbotsch has fixed bugs introduced by my PRs before, so it happens to everyone :)

@TIHan TIHan merged commit 941ec70 into dotnet:main Sep 2, 2022
@TIHan
Copy link
Contributor Author

TIHan commented Sep 2, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2980890529

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

@TIHan an error occurred while backporting to release/7.0, please check the run log for details!

Error: @TIHan is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=TIHan

@TIHan
Copy link
Contributor Author

TIHan commented Sep 2, 2022

@dotnet/jit-contrib Can someone run the backport release7.0? It says I'm not a collaborator at the moment.

@BruceForstall
Copy link
Member

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2981385893

@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants