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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:126
+
+      BasicBlock *Header = TheLoop->getHeader();
+      Function &F = *Header->getParent();
----------------
There are later checks that a header exists, so they are either unnecessary or the check should be performed here. You also check for the latch later, when that could also be done up front too.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:370
+
+    // We need a preheader, getIncomingValueForBlock assumes there is one.
+    if (!TheLoop->getLoopPreheader())
----------------
why is this performed inside the loop, should this function even called if this is false?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:441
+// Check if instruction I is a mul instruction or in its chain of operands.
+static bool IsInCandidates(Instruction *I, ParallelMACList &Candidates) {
+  for (auto &C : Candidates) {
----------------
This looks clumsy, shouldn't it be a method of ParallelMAC? You could also use SetVector instead of SmallVector to avoid the searching loop. I know that the vectors are likely to be small, but it would improve readability.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:458
+  for (auto &I : *Header) {
+    if (IsInCandidates(&I, Candidates))
+      continue;
----------------
Why are you adding all the instructions? You should only need to check ones that mayReadOrWriteMemory.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:474
+      assert(C.MemLocs.size() >= 2 && "expecting at least 2 memlocs");
+      for (unsigned i = 0; i < C.MemLocs.size(); i += 2) {
+        const auto &MemLoc0 = C.MemLocs[i+0];
----------------
Why process two MemLocs at a time?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:553
+  BasicBlock *Header = L->getHeader();
+  if (!Header)
+    return false;
----------------
Hmmm, now I'm wondering what happens if the header is not also the latch? I think the assumption is that it is.


https://reviews.llvm.org/D48128