[PATCH] D142482: [Codegen][ARM][AArch64] Support symmetric operations on complex numbers

[Dave Green via Phabricator via llvm-commits]

dmgreen added a reviewer: SjoerdMeijer.
dmgreen added a comment.

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.



================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:712-722
+  // Must be either a unary or binary op
+  if (!I->isUnaryOp() && !I->isBinaryOp())
+    return false;
+
+  // Must not have any side effects, or touch memory
+  if (I->mayHaveSideEffects() || I->mayReadOrWriteMemory())
+    return false;
----------------
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.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:744
+
+  auto *R0 = dyn_cast<Instruction>(Real->getOperand(0));
+  auto *I0 = dyn_cast<Instruction>(Imag->getOperand(0));
----------------
I may be missing it, but what checks that the two operations are the same?


================
Comment at: llvm/test/CodeGen/AArch64/complex-deinterleaving-mixed-cases.ll:369-372
+; CHECK-NEXT:    movi v3.2d, #0000000000000000
+; CHECK-NEXT:    fcmla v3.4s, v0.4s, v1.4s, #0
+; CHECK-NEXT:    fcmla v3.4s, v0.4s, v1.4s, #90
+; CHECK-NEXT:    fadd v0.4s, v3.4s, v2.4s
----------------
pthariensflame wrote:
> 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?
That sounds like a good change, but I think it could be handled generically by the backend.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142482/new/

https://reviews.llvm.org/D142482