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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Came here to say that this looks nice and would be worthy a mention in the release notes, so that can be added, I think.

Then I started reading the patch again a bit and couldn't help myself writing done some nits.



================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:16
+// complex instruction). This is performed by checking the real and imaginary
+// components in parallel, and tracking the data flow for each component while
+// following the operand pairs.
----------------
Nit: perhaps just omit `in parallel`? Or if you want to keep it, it could benefit from a clarification.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:25
+// caught via asserts.
+//
+//===----------------------------------------------------------------------===//
----------------
I wouldn't mind a few more words/sentences about the internal datastructures, the graph, nodes, etc.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:50
+    "enable-complex-deinterleaving",
+    cl::desc("Enable generation of complex deinterleaving instructions"),
+    cl::init(true), cl::Hidden);
----------------
Nit: remove `deinterleaving` from this description?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:59
+static bool isInterleavingMask(ArrayRef<int> Mask);
+/// Checks the given mask, and determines whether said mask is deinterleaving.
+///
----------------
Nit: newline


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:109
+  Instruction *Imag;
+  // Instructions that should only exist within this node, there should be no
+  // users of these instructions outside the node.
----------------
Can you say what these instructions could be?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:173
+  SmallVector<NodePtr> CompositeNodes;
+  SmallPtrSet<Instruction *, 16> AllInstructions;
+
----------------
I haven't looked carefully, but why do we also need to record all instructions here? I mean, the graph can be queried for existence of nodes/instructions, or is this bookkeeping duplication more efficient?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:181
+
+  NodePtr validateAndSubmitCompositeNode(NodePtr Node) {
+    CompositeNodes.push_back(Node);
----------------
Nit: no validation going on here, only insertion?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:181
+
+  NodePtr validateAndSubmitCompositeNode(NodePtr Node) {
+    CompositeNodes.push_back(Node);
----------------
SjoerdMeijer wrote:
> Nit: no validation going on here, only insertion?
Nit: no validation going on here, only insertion?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:239
+  /// current graph.
+  bool identifyNodes(Instruction *RootI);
+  /// Perform the actual replacement of the underlying instruction graph.
----------------
Nit: newline?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:312
+static bool isInterleavingMask(ArrayRef<int> Mask) {
+  if ((Mask.size() & 1) == 1)
+    return false;
----------------
Nit: can omit the comparison with 1.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:343
+  for (auto &I : *B) {
+    if (auto *SVI = dyn_cast<ShuffleVectorInst>(&I)) {
+      // Look for a shufflevector that takes separate vectors of the real and
----------------
Nit: I am a fan of reducing indentation, e.g.:

  auto *SVI = dyn_cast<ShuffleVectorInst>(&I);
  if (!SVI)
    continue;
  if (!isInterleavingMask(SVI->getShuffleMask())
    continue;

etc.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:367
+ComplexDeinterleavingGraph::NodePtr
+ComplexDeinterleavingGraph::identifyNodeWithImplicitAdd(
+    Instruction *Real, Instruction *Imag,
----------------
Nit: maybe I overlooked something, but this doesn't seem to use any state from the class so could be just a helper function?


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:467
+
+  NodePtr CommonNode = identifyNode(cast<Instruction>(PartialMatch.first),
+                                    cast<Instruction>(PartialMatch.second));
----------------
Nit: I am wondering if these casts here and below are necessary.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:483
+      ComplexDeinterleavingOperation::CMulPartial, Real, Imag);
+  Node->Rotation = Rotation;
+  Node->addOperand(CommonNode);
----------------
nit: maybe some or all of these node setting things can be done in a constructor.


================
Comment at: llvm/lib/CodeGen/ComplexDeinterleavingPass.cpp:539
+  if (!R0 || !R1 || !I0 || !I1) {
+    LLVM_DEBUG(dbgs() << "  - Mul Op not Instruction\n");
+    return nullptr;
----------------
Nits: 
Op -> Operand,
not Instruction -> not a Instruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114174