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

[Nicholas Guy via Phabricator via llvm-commits]

NickGuy added inline comments.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:437
+
+  CommonOperandI = dyn_cast<Instruction>(CommonOperand);
+
----------------
dmgreen wrote:
> Should this function be calling identifyNode on the inputs, similar to identifyPartialMul?
Not here, no. The inputs need to be resolved as part of the whole pair, so the relevant one for the created node is passed back via CommonOperandI.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:472
+
+  if (RealI->getOpcode() != Instruction::FMul) {
+    CR = RealI->getOperand(0);
----------------
dmgreen wrote:
> When can this be false?
In it's current state, never. It was possible before moving to the implicitAdd approach


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:589
+
+  Node->Operands.push_back(RealI->getOperand(0));
+  Node->Operands.push_back(RealI->getOperand(1));
----------------
dmgreen wrote:
> I think I was expecting Operands to be the ComplexDeinterleavingCompositeNode* from identifyNode. That avoids the need to find the again later.
Attempting to plumb something together that acceps both `ComplexDeinterleavingCompositeNode*` and `Value*` caused more bloat and made things more difficult to follow. And we can't simply use `Node->ReplacementNode` as the `Value*` because it hasn't been assigned yet.
Because of that constraint, I'm opting to find the nodes later, though I'm open to ideas.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:775-776
+
+  auto TTI =
+      TL->getTargetMachine().getTargetTransformInfo(*RootValue->getFunction());
+
----------------
dmgreen wrote:
> 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).
Good catch, removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174