Cleanup of Complex Deinterleaving pass (NFCI)

[NickGuy]

Summary

General cleanup of ComplexDeinterleavingPass; adding more correct checks to make the pass more tolerant, and updating comments to be more accurate.

[dmgreen]

llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp

642–656

I think I find the existing code easier to follow and read. And it is more explicit about matching adds. Neither are super pretty, but I think I would recommend keeping what was there before.

857

This could actually be part of the other patch, if it the first part to introduce single element nodes.

[dmgreen]

This LGTM but please make sure the correct parts are in the correct patches. I don't think this will actually build on its own.

llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp

817

I think this is in the wrong patch now?

[dmgreen]

If you can move the identifySymmetricOperation call to the other patch then what remains LGTM. Thanks.

llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp

for (const auto &Node : CompositeNodes)
       Node->dump(OS);
   }
 
   /// Returns false if the deinterleaving operation should be cancelled for the
   /// current graph.
   bool identifyNodes(Instruction *RootI);
 
   /// Perform the actual replacement of the underlying instruction graph.
-  /// Returns false if the deinterleaving operation should be cancelled for the
-  /// current graph.
   void replaceNodes();
 };
 
 class ComplexDeinterleaving {
 public:
   ComplexDeinterleaving(const TargetLowering *tl, const TargetLibraryInfo *tli)
       : TL(tl), TLI(tli) {}
   bool runOnFunction(Function &F);
std::pair<Instruction *, Instruction *> PartialMatch(
       (Rotation == ComplexDeinterleavingRotation::Rotation_0 ||
        Rotation == ComplexDeinterleavingRotation::Rotation_180)
           ? CommonOperand
           : nullptr,
       (Rotation == ComplexDeinterleavingRotation::Rotation_90 ||
        Rotation == ComplexDeinterleavingRotation::Rotation_270)
           ? CommonOperand
           : nullptr);
-  NodePtr CNode = identifyNodeWithImplicitAdd(
-      cast<Instruction>(CR), cast<Instruction>(CI), PartialMatch);
+
+  auto *CRInst = dyn_cast<Instruction>(CR);
+  auto *CIInst = dyn_cast<Instruction>(CI);
+
+  if (!CRInst || !CIInst) {
+    LLVM_DEBUG(dbgs() << "  - Common operands are not instructions.\n");
+    return nullptr;
+  }
+
+  NodePtr CNode = identifyNodeWithImplicitAdd(CRInst, CIInst, PartialMatch);
   if (!CNode) {
     LLVM_DEBUG(dbgs() << "  - No cnode identified\n");
     return nullptr;
   }
 
   NodePtr UncommonRes = identifyNode(UncommonRealOp, UncommonImagOp);
   if (!UncommonRes) {
     LLVM_DEBUG(dbgs() << "  - No UncommonRes identified\n");
ComplexDeinterleavingGraph::identifyPartialMul(Instruction *Real,
   Node->addOperand(CNode);
   return submitCompositeNode(Node);
 }
 
 ComplexDeinterleavingGraph::NodePtr
 ComplexDeinterleavingGraph::identifyAdd(Instruction *Real, Instruction *Imag) {
   LLVM_DEBUG(dbgs() << "identifyAdd " << *Real << " / " << *Imag << "\n");
 
-  // Determine rotation
-  ComplexDeinterleavingRotation Rotation;
-  if ((Real->getOpcode() == Instruction::FSub &&
-       Imag->getOpcode() == Instruction::FAdd) ||
-      (Real->getOpcode() == Instruction::Sub &&
-       Imag->getOpcode() == Instruction::Add))
-    Rotation = ComplexDeinterleavingRotation::Rotation_90;
-  else if ((Real->getOpcode() == Instruction::FAdd &&
-            Imag->getOpcode() == Instruction::FSub) ||
-           (Real->getOpcode() == Instruction::Add &&
-            Imag->getOpcode() == Instruction::Sub))
-    Rotation = ComplexDeinterleavingRotation::Rotation_270;
-  else {
-    LLVM_DEBUG(dbgs() << " - Unhandled case, rotation is not assigned.\n");
+  unsigned RotKey = 0;
+  RotKey |= Real->getOpcode() == Instruction::FSub ||
+            Real->getOpcode() == Instruction::Sub;
+  RotKey |= ((Imag->getOpcode() == Instruction::FSub ||
+              Imag->getOpcode() == Instruction::Sub)
+             << 1);
+  if ((RotKey & 2) == 2)
+    RotKey ^= 1;
+
+  ComplexDeinterleavingRotation Rotation =
+      (ComplexDeinterleavingRotation)RotKey;
+
+  if (Rotation == llvm::ComplexDeinterleavingRotation::Rotation_0 ||
+      Rotation == llvm::ComplexDeinterleavingRotation::Rotation_180) {
+    LLVM_DEBUG(dbgs() << " - Unsupported rotation.\n");
     return nullptr;
   }
 
   auto *AR = dyn_cast<Instruction>(Real->getOperand(0));
   auto *BI = dyn_cast<Instruction>(Real->getOperand(1));
   auto *AI = dyn_cast<Instruction>(Imag->getOperand(0));
   auto *BR = dyn_cast<Instruction>(Imag->getOperand(1));
 
ComplexDeinterleavingGraph::identifyNode(Instruction *Real, Instruction *Imag) {
   }
 
   if (TL->isComplexDeinterleavingOperationSupported(
           ComplexDeinterleavingOperation::CAdd, NewVTy) &&
       isInstructionPairAdd(Real, Imag)) {
     return identifyAdd(Real, Imag);
   }
 
   return nullptr;
 }
 
 bool ComplexDeinterleavingGraph::identifyNodes(Instruction *RootI) {
   Instruction *Real;
   Instruction *Imag;
   if (!match(RootI, m_Shuffle(m_Instruction(Real), m_Instruction(Imag))))
     return false;
 
   RootValue = RootI;
   AllInstructions.insert(RootI);
   RootNode = identifyNode(Real, Imag);
 
   LLVM_DEBUG({
     Function *F = RootI->getFunction();
     BasicBlock *B = RootI->getParent();
     dbgs() << "Complex deinterleaving graph for " << F->getName()
            << "::" << B->getName() << ".\n";
     dump(dbgs());
     dbgs() << "\n";
   });
 
   // Check all instructions have internal uses
   for (const auto &Node : CompositeNodes) {
     if (!Node->hasAllInternalUses(AllInstructions)) {
-      LLVM_DEBUG(dbgs() << "  - Invalid internal uses\n");
+      LLVM_DEBUG(dbgs() << "  - Invalid internal uses in " << Node.get()
+                        << "\n");
       return false;
     }
   }
   return RootNode != nullptr;
 }
 
 Value *ComplexDeinterleavingGraph::replaceNode(
     ComplexDeinterleavingGraph::RawNodePtr Node) {
   if (Node->ReplacementNode)
     return Node->ReplacementNode;
 
   Value *Input0 = replaceNode(Node->Operands[0]);
-  Value *Input1 = replaceNode(Node->Operands[1]);
+  Value *Input1 =
+      Node->Operands.size() > 1 ? replaceNode(Node->Operands[1]) : nullptr;
   Value *Accumulator =
       Node->Operands.size() > 2 ? replaceNode(Node->Operands[2]) : nullptr;
 
+  if (Input1)
     assert(Input0->getType() == Input1->getType() &&
            "Node inputs need to be of the same type");
 
   Node->ReplacementNode = TL->createComplexDeinterleavingIR(
       Node->Real, Node->Operation, Node->Rotation, Input0, Input1, Accumulator);
 
   assert(Node->ReplacementNode && "Target failed to create Intrinsic call.");
   NumComplexTransformations += 1;
   return Node->ReplacementNode;
 }