This is an archive of the discontinued LLVM Phabricator instance.

[Codegen] Support symmetric operations on complex numbers
ClosedPublic

Authored by NickGuy on Jan 24 2023, 8:49 AM.

Details

Summary

This provides support for symmetric operations being performed on complex numbers, allowing for such patterns as o[i] = (x[i] * y[i]) + z[i]; and z[i] += x[i] * y[i] (where the expression isn't rearranged beyond the recognised patterns)

Diff Detail

Event Timeline

NickGuy created this revision.Jan 24 2023, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 8:49 AM
NickGuy requested review of this revision.Jan 24 2023, 8:49 AM

Sounds like a nice idea. It might be better not to represent it as "complex rotate with zero rotation", but as a binary operation that is applied equally to both real and imaginary values. Possibly a new ComplexDeinterleavingOperation type? It could then apply to any binary operator, so if you have R=fsub(a,b) and I=fsub(c,d), and it is valid to transform with identifyNode(a,c) and identifyNode(b,d), then it is valid to use a fsub on the interleaved version. It could run at the end of ComplexDeinterleavingGraph::identifyNode - if we haven't found anything else so far and the operations are equal binary operators then try and recurse into the operands. There are some unary operations too that might apply, like fneg and llvm.fabs intrinsics. I'm not sure how many of them commonly come up with complex numbers, but hopefully once some are added the others can be fairly simple additions.

NickGuy updated this revision to Diff 493952.Feb 1 2023, 8:07 AM
NickGuy retitled this revision from [CodeGen][ARM][AArch64] Support complex additions with no rotation to [Codegen][ARM][AArch64] Support symmetric operations on complex numbers.
NickGuy edited the summary of this revision. (Show Details)

I've changed the approach to not be so focused on "complex addition with zero rotation", and instead common operations applied to both sides (dubbed "symmetric operations"). The current implementation doesn't allow for targets to define their own supported operations, but also I'm not sure if there are many (if any at all) operations that wouldn't be common.

pthariensflame added inline comments.
llvm/test/CodeGen/AArch64/complex-deinterleaving-mixed-cases.ll
355–507

Would it not be better to recognize the addend as part of the mla, thus saving an fadd? Or is that out of scope for this patch, or wrong for some subtle reason?

Sounds good. A few high level points:

  • Some of the changes here look unrelated to these new operations. Can they be split out into a separate change.
  • Add more tests for all the various instructions added so far.
  • The creation of the new nodes doesn't need to happen in Arm/AArch64. It can be shared between the two in the pass.
  • I'm not sure about the name Passthrough (a passthrough of what?), but I'm not sure I have a better suggestion. Maybe CommonOperation or just BinOp (even if they can also be UnaryOps)? It's not a very important point, but it is worth adding comments to explain the node type.
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
707–717

This checks various conditions about the operation, then checks for exact opcodes below, none of which will throw of have side effects. The switch table should be enough.

739

I may be missing it, but what checks that the two operations are the same?

llvm/test/CodeGen/AArch64/complex-deinterleaving-mixed-cases.ll
355–507

That sounds like a good change, but I think it could be handled generically by the backend.

NickGuy updated this revision to Diff 494287.EditedFeb 2 2023, 6:36 AM
NickGuy retitled this revision from [Codegen][ARM][AArch64] Support symmetric operations on complex numbers to [Codegen] Support symmetric operations on complex numbers.

Split out unrelated changes, added more tests, unified the common logic, and renamed Passthrough to Symmetric.

This checks various conditions about the operation, then checks for exact opcodes below, none of which will throw of have side effects. The switch table should be enough.

It was an initial attempt to try and be clever, removed.

I may be missing it, but what checks that the two operations are the same?

You're not missing anything, it existed at one point, but it got removed in one of the iterations. I've readded it.

dmgreen added inline comments.Feb 6 2023, 12:52 AM
llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp
643

This needn't be here.

720

Formatting

852

This could be in the other cleanup patch.

907

Formatting.

NickGuy updated this revision to Diff 503370.Mar 8 2023, 7:46 AM
dmgreen accepted this revision.Mar 10 2023, 4:35 AM

If you can move the call to identifySymmetricOperation into this patch, then it LGTM. Cheers

This revision is now accepted and ready to land.Mar 10 2023, 4:35 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 5:11 AM
This revision was automatically updated to reflect the committed changes.