[PATCH] D48128: [ARM] Parallel DSP IR Pass

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:131-133
+      auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
+      if (!TPC)
+        return false;
----------------
This looks odd. Why aren't you marking TargetPassConfig as required and getting it using getAnalysis?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:237-241
+    if (match(V0, m_APInt(C0)) && match(V1, m_APInt(C1))) {
+      if (C0 != C1)
+        return false;
+    } else
+      return false;
----------------
I think this can be simplified to

```
if (!(match(V0, m_APInt(C0)) && match(V1, m_APInt(C1)) && C0 == C1))
  return false;
```



================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:306
+      if (!AreSymmetrical(VL0, VL1))
+        return false;
+
----------------
Why return false here, when you have continues for failing checks above?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:330-332
+          PMACPair P = std::make_pair(&PMul0, &PMul1);
+          R.PMACPairs.push_back(P);
+          return true;
----------------
Given that we return true after finding a pair, doesn't that mean that we only ever have one entry in PMACPairs? If so we don't need it, and this function can just return the pair, or maybe we should do the call to CreateSMLADCall here.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:386-388
+    BasicBlock *Latch = TheLoop->getLoopLatch();
+    if (!Latch)
+      continue;
----------------
This should be outside the loop, and cause the function to return false if there's no loop latch.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:392
+    if (!Acc)
+      return false;
+
----------------
Why return false here instead of continuing?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:407
+
+static void AddCandidateMAC(Reduction &R, const Instruction *Acc,
+                            Value *MulOp0, Value *MulOp1, int MulOpNum) {
----------------
This function only manipulates the candidates list, so it should take that as an argument not Reduction.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:495
+  ReductionList Reductions;
+  ParallelMACList ParallelMACs;
+
----------------
This is unused.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:497-500
+  if (MatchReductions(F, L, Header, Reductions)) {
+    MatchParallelMACs(Reductions);
+    return CreateParallelMACs(Reductions);
+  }
----------------
It would be more understandable what's going on if you were more explicit here about what the inputs and outputs to each function are rather than hiding everything inside the Reductions list. If you restructured it to be more like:
```
Changed = false;
Reductions = MatchReductions(F, L, Header);
for (auto &R : Reductions) {
  Candidates = MatchParallelMACs(R);
  Changed = CreateParallelMACs(Candidates) || Changed;
}

```
then it would be easier to understand.


https://reviews.llvm.org/D48128