[PATCH] D114174: [ARM][CodeGen] Add support for complex deinterleaving

[Nicholas Guy via Phabricator via llvm-commits]

NickGuy added a comment.

Forgot to submit the comments adjoining the patch...



================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:786-787
+    // Type mismatch
+    if (CN->Input0->getType() != CN->Input1->getType())
+      return false;
+
----------------
dmgreen wrote:
> When can this return false?
It can be false if a shuffle hasn't been traversed properly. In hindsight though, this could be an assertion instead as it indicates a problem with the node construction.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:568-570
+      if (!NodeCreated)
+        identifyNode(cast<Instruction>(Pair.Imaginary),
+                     cast<Instruction>(Pair.Real), TL, _);
----------------
dmgreen wrote:
> Is it valid to test the operands in the opposite order?
No, but it's also not invalid (the checks do what they're supposed to and don't produce a node). I've removed these.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:646
+                                         const TargetLowering *TL,
+                                         Instruction *&CommonOperandI) {
+  auto IsZeroInitializer = [&](Value *V) -> bool { return match(V, m_Zero()); };
----------------
dmgreen wrote:
> CommonOperandI only seems to be important between identifyPartialMul and identifyNodeWithImplicitAdd
It used to be needed all through, but that is no longer the case. Changed the signatures and such to reflect this.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:723-731
+  if (getOperatingComponentOfValue(RootI->getOperand(0)) !=
+      OperatingComponent::Real) {
+    return false;
+  }
+
+  if (getOperatingComponentOfValue(RootI->getOperand(1)) !=
+      OperatingComponent::Imaginary) {
----------------
dmgreen wrote:
> I feel that this should be correct by construction. What cases are not correct?
The one that is easiest to see is when attempting to multiply by a value rotated by 270 (`a[i] * (b[i] * I*I*I)`), it produces the following IR which has the real and imaginary components reversed at %interleaved.vec

```
  %a.real = shufflevector <8 x float> %a, <8 x float> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
  %a.imag = shufflevector <8 x float> %a, <8 x float> poison, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
  %b.real = shufflevector <8 x float> %b, <8 x float> poison, <4 x i32> <i32 0, i32 2, i32 4, i32 6>
  %b.imag = shufflevector <8 x float> %b, <8 x float> poison, <4 x i32> <i32 1, i32 3, i32 5, i32 7>
  %0 = fmul fast <4 x float> %b.imag, %a.imag
  %1 = fmul fast <4 x float> %b.real, %a.real
  %2 = fsub fast <4 x float> %0, %1
  %3 = fmul fast <4 x float> %b.imag, %a.real
  %4 = fmul fast <4 x float> %b.real, %a.imag
  %5 = fadd fast <4 x float> %4, %3
  %interleaved.vec = shufflevector <4 x float> %5, <4 x float> %2, <8 x i32> <i32 0, i32 4, i32 1, i32 5, i32 2, i32 6, i32 3, i32 7>
```


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:790-792
+    // Discover the relevant inputs for a given node.
+    // This can't be moved any sooner, as the inputs could be the replacement
+    // value of a previous node.
----------------
dmgreen wrote:
> I don't think this should be needed. The inputs should just be present from the ReplacementNode of the operands.
It's more of a side effect of analysing the whole tree before actually performing the replacement (which we do in case the tree can't be transformed). With some of the recent changes however, this function can't return false so it's merely a step of the replacement now.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:810
+    // what it might be based on the vector width
+    // TODO Defer to the target for this info. Do we care enough?
+    auto *VTy = cast<FixedVectorType>(CN->Input0->getType());
----------------
dmgreen wrote:
> If the statistic is being awkward then it is probably not worth keeping (or keeping simple - just counting number of transforms, not the number of individual intrinsics that might become in the backend).
Changed to track the transforms rather than individual intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174