This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Extend ComplexDeinterleaving pass to recognise patterns using integer types
ClosedPublic

Authored by igor.kirillov on Jun 26 2023, 12:48 PM.

Details

Summary

AArch64 introduced CMLA and CADD instructions as part of SVE2. This
change allows to generate such instructions when this architecture
feature is available.

Diff Detail

Event Timeline

igor.kirillov created this revision.Jun 26 2023, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 12:48 PM
igor.kirillov requested review of this revision.Jun 26 2023, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 12:48 PM
igor.kirillov retitled this revision from [CodeGen] Add support for integers using SVE2 in ComplexDeinterleaving pass Depends on D153355 to [CodeGen] Add support for integers using SVE2 in ComplexDeinterleaving passDepends on D153355.Jun 27 2023, 3:24 AM
mgabka added inline comments.Jun 27 2023, 3:35 AM
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.
It would make the code more logical, i.e if fp propagate flag and set to fadd.
Also I recall that this pass can match things produced by -O3, are the flags set in that case as well?

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

igor.kirillov marked 3 inline comments as done.
  • IsOperationsupported -> IsOperationSupported
  • Remove some artefacts
igor.kirillov added inline comments.Jun 27 2023, 6:22 AM
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.

igor.kirillov added inline comments.Jun 27 2023, 6:29 AM
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:
https://reviews.llvm.org/D153862

1026–1028

That is part of integer support functionality, namely when we have a Neg operation, for example, c = - a * b;

mgabka added inline comments.Jul 13 2023, 5:44 AM
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

Address comments

igor.kirillov marked 7 inline comments as done.

Move comments from definition to declaration

mgabka accepted this revision.Jul 18 2023, 1:16 AM

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

This revision is now accepted and ready to land.Jul 18 2023, 1:16 AM
igor.kirillov retitled this revision from [CodeGen] Add support for integers using SVE2 in ComplexDeinterleaving passDepends on D153355 to [CodeGen] Extend ComplexDeinterleaving pass to recognise patterns using integer types.
igor.kirillov edited the summary of this revision. (Show Details)

Update commit message and some tests comments

This revision was landed with ongoing or failed builds.Jul 19 2023, 4:05 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Sep 5 2023, 3:05 PM

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

(Ref https://crbug.com/1473604)