AArch64 introduced CMLA and CADD instructions as part of SVE2. This
change allows to generate such instructions when this architecture
feature is available.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp | ||
---|---|---|
958 | probably should start with capital S | |
976–982 | wouldn't be more clear to guard it by if(isa<FPMathOperator>(Real)) ? on the same from do we need to have a check somewhere that Real and Imag are of the same type? i.e both fp or integer? | |
1026–1028 | looking at the diff this looks like a new functionality, maybe worth to add it as a separate patch? | |
1365–1370 | in my opinion it would make more sense to distinguishing between fp and int operations based on the original instructions, not based on extra flags we want to propagate. how difficult it would be to change the logic to the one I propose? | |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
25949 | the IsInt check he is redundant here as it is already covered above |
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp | ||
---|---|---|
1365–1370 | FFastMathOperation always has FFastFlags; any Integer operation doesn't have them. So, I have to choose between having two variables (bool HasFastFlags, FFastFlags Flags) or having only one variable that incorporates this information (std::optional<FFastFlags>). I suggest using the second approach and reducing the number of entities. | |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
25949 | Yes, that's a rudiment of my first attempt to handle the case. But integer cmla doesn't accept a mask as an argument. |
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp | ||
---|---|---|
976–982 | By design, they should not have different types, but after reading your comment, I realised I missed a check inside identifyReductions. So, I created a patch to fix that: | |
1026–1028 | That is part of integer support functionality, namely when we have a Neg operation, for example, c = - a * b; |
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp | ||
---|---|---|
103 | please add a 1 line comment (just to emphasise that it works with floats and integers) | |
105 | please add a comment | |
531 | I think you can use here m_Neg pattern matches, documentation for it says: Matches a "Neg" as 'sub 0, V' | |
970 | looks like it can be deleted as we have an assignment after the check below. | |
1882 | wouldn't be easier to always set FatMathFlags if they exist, outside of this switch? | |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
25973 | I think you can just use here getAllOnesValue | |
llvm/test/CodeGen/AArch64/complex-deinterleaving-i16-add-scalable.ll | ||
7 | worth to mention why |
Probably worth to polish a bit commit message, but apart from that it LGTM.
Thanks for addressing my comments.
llvm/test/CodeGen/AArch64/complex-deinterleaving-i16-mul-scalable.ll | ||
---|---|---|
7 | Could you add a comment explaining why? (I thought that you are going to apply my comment to the previous test to all tests). | |
llvm/test/CodeGen/AArch64/complex-deinterleaving-i8-add-scalable.ll | ||
8 | please add a comment explaining why |
If anyone bisects an assert with -fprofile-generate (or, if you build without asserts, invalid pgo profiles) on aarch64 to this change: you're correct, and the fix for that was D159209
please add a 1 line comment (just to emphasise that it works with floats and integers)