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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

OK great. It would be good to see a lot of test cases for pretty much anything that might go wrong. Things like the shuffles with incorrect masks, fadd's where there should be fsubs, patterns that cross basic blocks, any of the conditions that can currently return false that we can test.



================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:127
+
+  struct IdentifyResult {
+    bool IsValid;
----------------
Can we just use nullptr as the sentinel value for IsValid = false, removing the need for this struct? If we need some way to represent the last shuffle node then we could add a Shuffle or Leaf node type (which could itself have a ReplacementNode just set to the shuffle->operand(0)).

Hopefully that also simplifies resolveInputs / Elevate. Especially if Accumulator is treated in the same way as an Operand. Hopefully that is simpler.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:437
+
+  CommonOperandI = dyn_cast<Instruction>(CommonOperand);
+
----------------
Should this function be calling identifyNode on the inputs, similar to identifyPartialMul?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:472
+
+  if (RealI->getOpcode() != Instruction::FMul) {
+    CR = RealI->getOperand(0);
----------------
When can this be false?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:589
+
+  Node->Operands.push_back(RealI->getOperand(0));
+  Node->Operands.push_back(RealI->getOperand(1));
----------------
I think I was expecting Operands to be the ComplexDeinterleavingCompositeNode* from identifyNode. That avoids the need to find the again later.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:716
+
+  if (getOperatingComponentOfValue(RootI->getOperand(0)) !=
+      OperatingComponent::Real) {
----------------
These seem to do recursive checks into the operands, but that is already being done again in identifyNode. Can we just remove them and rely on identifyNode?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:775-776
+
+  auto TTI =
+      TL->getTargetMachine().getTargetTransformInfo(*RootValue->getFunction());
+
----------------
This isn't used any more, which is good (I think there might be a better way to get a TTI if it was needed).


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:786-787
+    // Type mismatch
+    if (CN->Input0->getType() != CN->Input1->getType())
+      return false;
+
----------------
When can this return false?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174