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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

The tests are good, but they currently only cover each of the types for vcadd and vcmla0/90. I think we need more to show all the edgecases that should or should not be matched. They are not needed for all types, but we should try and make examples of each.



================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:162
+
+  Instruction *ConvergingI;
+  SmallVector<NodePtr> CompositeNodes;
----------------
ConvergingI is the Root of the graph? Perhaps just call it RootValue.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:528
+
+  Value *CommonOperand =
+      getCommonValues({RealMulI->getOperand(0), RealMulI->getOperand(1),
----------------
What if the operands are FMul(A, A) and FMul (B, C)?
I think it needs to be more precise about which nodes are which. 


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:563
+    // If no node was found, check the other operand permutation
+    if (!CNode)
+      CNode = identifyNodeWithImplicitAdd(
----------------
I don't think it makes sense to treat these backwards. We know the CR part should be real and the CI is imaginary. It would only be valid to treat them the other way around if there was some sort of shuffle added.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:592
+
+  return Node;
+}
----------------
Does this need to be calling identifyNode on the third argument too? The one derived from UncommonValues.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:655
+
+  if (auto *Shuffle = dyn_cast<ShuffleVectorInst>(RealI)) {
+    auto *Op1 = Shuffle->getOperand(1);
----------------
This looks like it needs to match:
 - RealI and ImagI are both shuffles.
 - They both have the same Operand0
 - They both have "deinterleaving" masks.
I don't think the type of the value of Operand0 otherwise matters. It doesn't matter if it is a Load or a Argument, we can always just use it.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:748
+      Value *W = V;
+      static const int RealInterleaveInts[] = {0,  2,  4,  6,  8,  10, 12, 14,
+                                               16, 18, 20, 22, 24, 26, 28, 30};
----------------
It's best not to use static data like this, we can make it more generic. The match can be `m_Shuffle(..., m_Mask(Mask))`, then check that the Mask is an isDeinterleavingMask. It does need to check _which_ deinterleaving mask it is though for the Real/Imaginary parts.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:853
+    // value of a previous node.
+    if (!wrangleInputs(CN))
+      return false;
----------------
If the Operands that CN depends on are included in the Node, then we can just walk up the tree making sure we create the Operands before the Nodes that use them, using the Value's that the operands produce as the Input0/Input1/Accumulator below.
That avoids the need to "wrangle" any inputs at this stage, as we already know the nodes we need.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:868
+
+    CostOfIntrinsics += TTI.getInstructionCost(
+        cast<Instruction>(CN->ReplacementNode), CostKind);
----------------
It might be simpler to remove anything Cost based from this revision, adding it back in if it is needed in followup patches.
All the identification that happens in this patch should always be cheaper, as far as I understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174