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

[ARM][ISel] Fix crash of ISD::FMINNUM/FMAXNUM #65849

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

vfdff
Copy link
Contributor

@vfdff vfdff commented Sep 9, 2023

The instruction of ISD::FMINNUM/FMAXNUM should be legal if HasFPARMv8 && HasNEON.
For the combination of armv7+fp-armv8, armv7 imply the feature HasNEON
on, and fp-armv8 matchs the feature HasFPARMv8, so it is legal

Fixes #65820

@vfdff vfdff requested a review from a team as a code owner September 9, 2023 12:28
@davemgreen
Copy link
Collaborator

I think it should be the other way around. The instruction should be legal if HasFPARMv8 && HasNEON. It's a bit of a strange combination to have armv7+fp-armv8, but that appears to be what happens for the scalar instructions and from gcc.

@vfdff
Copy link
Contributor Author

vfdff commented Sep 11, 2023

I think it should be the other way around. The instruction should be legal if HasFPARMv8 && HasNEON. It's a bit of a strange combination to have armv7+fp-armv8, but that appears to be what happens for the scalar instructions and from gcc.

Thanks @davemgreen. Would you please show how can we infer that the instruction fmaxnum should be legal if HasFPARMv8 && HasNEON ?
The related pattern is defined with Requires<[HasV8, HasNEON]>, so the instruction should be legal iff HasV8 && HasNEON. Meanwhile, the feature HasV8 and HasFPARMv8 is independent from the definition of ARMv8a.

def NEON_VMAXNMNQf  : N3VQIntnp<0b00110, 0b00, 0b1111, 1, 1,
                                  N3RegFrm, NoItinerary, "vmaxnm", "f32",
                                  v4f32, v4f32, fmaxnum, 1>,
                                  Requires<[HasV8, HasNEON]>;

def ARMv8a    : Architecture<"armv8-a",   "ARMv8a",   [HasV8Ops,
                                                       FeatureFPARMv8,
                                                       FeatureNEON,
                                                       ...]

@davemgreen
Copy link
Collaborator

Yeah can we change it to [HasFPARMv8, HasNEON]?

@vfdff
Copy link
Contributor Author

vfdff commented Sep 12, 2023

Yeah can we change it to [HasFPARMv8, HasNEON]?
Apply your comment, thanks!

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Other than the comments, this LGTM. Thanks for working on this.

llvm/test/CodeGen/ARM/minnum-maxnum-intrinsics.ll Outdated Show resolved Hide resolved
llvm/lib/Target/ARM/ARMISelLowering.cpp Outdated Show resolved Hide resolved
The instruction of ISD::FMINNUM/FMAXNUM should be legal if HasFPARMv8 && HasNEON.
For the combination of armv7+fp-armv8, armv7 imply the feature HasNEON
on, and fp-armv8 matchs the feature HasFPARMv8, so it is legal.

Fixes llvm#65820
Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks. Some of the tests are showing some worse scalization but for what you are fixing this LGTM. Thanks for the fix.

@vfdff vfdff merged commit 347b3f1 into llvm:main Sep 14, 2023
2 checks passed
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Sep 14, 2023
The instruction of ISD::FMINNUM/FMAXNUM should be legal if HasFPARMv8 &&
HasNEON.
For the combination of armv7+fp-armv8, armv7 imply the feature HasNEON
on, and fp-armv8 matchs the feature HasFPARMv8, so it is legal

Fixes llvm#65820
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
The instruction of ISD::FMINNUM/FMAXNUM should be legal if HasFPARMv8 &&
HasNEON.
For the combination of armv7+fp-armv8, armv7 imply the feature HasNEON
on, and fp-armv8 matchs the feature HasFPARMv8, so it is legal

Fixes llvm#65820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot select: 0x55848e4be0e0: v4f32 = fmaxnum 0x55848fa99830, 0x55848fc54190
3 participants