This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][CodeGen] Add AArch64 support for complex deinterleaving
ClosedPublic

Authored by NickGuy on Jul 4 2022, 2:56 AM.

Diff Detail

Event Timeline

NickGuy created this revision.Jul 4 2022, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 2:56 AM
NickGuy requested review of this revision.Jul 4 2022, 2:56 AM
NickGuy updated this revision to Diff 442050.

Added context missing from initial patch

dmgreen added inline comments.Jul 6 2022, 2:41 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
23293

I think ComplxNum requires NEON, so checking for the first should be enough.

23340

There is quite a lot of debug message left here. I assume it will be removed in the final version?

llvm/test/CodeGen/AArch64/ComplexArithmetic/complex-arithmetic-f16-add.ll
1 ↗(On Diff #442050)

These don't need to be in a "ComplexArithmetic" directory - they are already easy to identify from the file prefix.

6 ↗(On Diff #442050)

Can you try and clean up these newlines too. There are gaps in places, but some of the functions are a little jammed together.

NickGuy updated this revision to Diff 449950.Aug 4 2022, 6:16 AM
NickGuy marked 4 inline comments as done.

Addressed comments, cleaned up debug outputs, and implemented vector splitting

NickGuy updated this revision to Diff 473969.Nov 8 2022, 5:40 AM

Addressed comments, and rebased to align with latest revision of D114174 (Diff 16)

NickGuy marked an inline comment as not done.Nov 8 2022, 5:41 AM

Can you add tests cases for some of the awkward cases we found in D114174, like the <12 x > vectors (if they are not present already).

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
23292

Can you add some extra newlines between methods

23293

This I don't think needs the NEON check.

23308

Formatting, here and below.

23323

Add a message to the assert

Matt added a subscriber: Matt.Nov 14 2022, 9:54 AM
NickGuy updated this revision to Diff 475439.Nov 15 2022, 5:47 AM
NickGuy marked 5 inline comments as done.

Addressed comments, and added more tests

SjoerdMeijer added inline comments.Nov 15 2022, 6:34 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
23330

It isn't clear to me why TyWidth can have any value greater than 128. Don't we expect it to be a multiple of something?

23344

Accumulator always has a value at this point, don't need to check it?

NickGuy updated this revision to Diff 475466.Nov 15 2022, 7:10 AM
NickGuy marked 2 inline comments as done.
NickGuy added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
23330

We do, but it's also backed up by the if ((VTyWidth < 128 && VTyWidth != 64) || !llvm::isPowerOf2_32(VTyWidth)) above. Though changing the assert to reflect this restriction might be best, rather than assuming that it's met.

23344

This check is needed, however the previous assignment is not. I've moved it down to where it is needed

Cool, thanks.
Looks like a nice change to me. Will leave further reviews/approval up to Dave.

dmgreen accepted this revision.Nov 16 2022, 2:06 AM

I was hoping we could get some correctness results for the MVE side overnight, but out internal testing seems pretty broken for other reasons. I don't see anything that is complex number related, so hopefully this is OK.

So long as you have done some testing, LGTM.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
23324–23325

Formatting, and it probably wants brackets around the ||

This revision is now accepted and ready to land.Nov 16 2022, 2:06 AM
This revision was landed with ongoing or failed builds.Nov 16 2022, 6:01 AM
This revision was automatically updated to reflect the committed changes.