[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:168
+  bool tryIdentifyNodeFromPair(Value *RealV, Value *ImagV) {
+    auto Pair = getOperandPair(RealV, ImagV);
+    if (Pair.Valid) {
----------------
dmgreen wrote:
> Why is this needed? When are the Real and Imag values not already correct? It seems strange to be able to reverse them.
It was a holdover function that I reused for the `.Valid` check. It's not used anywhere else so I've removed it (and the relevant struct), and moved the relevant check to here.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:589
+
+  Node->Operands.push_back(RealI->getOperand(0));
+  Node->Operands.push_back(RealI->getOperand(1));
----------------
dmgreen wrote:
> NickGuy wrote:
> > 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.
> I was expecting it to just accept ComplexDeinterleavingCompositeNode. If all the operands are ComplexDeinterleavingCompositeNode, then they just need to be visited in the correct order to assign ReplacementNode based on how they need to be transformed. (Which I believe is fine providing we visit them in reverse order, like is already done).
> 
> i.e. we construct a graph made up of ComplexDeinterleavingCompositeNode, then transform that graph. That seems like a simpler, more extensible design going forward. It gets more complicated if there is a mixing of values between the original IR and the newly constructed intrinsics. I could imagine that might make graphs (as opposed to DAGs) more difficult, but we don't yet support any cycles.
I've converted the pseudo-graph (a list with lookups) to an actual graph structure, with every node being a ComplexDeinterleavingCompositeNode*, meaning that we're no longer mixing `Value*`s and `ComplexDeinterleavingCompositeNode*`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174