[PATCH] D22630: Loop rotation

Wed Dec 28 13:44:23 PST 2016

mzolotukhin requested changes to this revision.
mzolotukhin added a comment.
This revision now requires changes to proceed.

Hi,

Please find some remarks inline. Also, some remarks from previous reviews haven't been addressed yet.

Thanks,
Michael

================
Comment at: llvm/lib/IR/Metadata.cpp:1109-1112
+void MDAttachmentMap::getAllIDs(SmallVectorImpl<unsigned> &Result) const {
+  for (auto &A : Attachments)
+    Result.push_back(A.first);
+}
----------------
Can we remove this function and use `SmallVector::append` instead?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:18-21
+// The loop is an SEME (Single Entry Multiple Exit) region.
+// The loop has a header and its terminator conditionally exits the loop
+// The loop has a latch and its terminator does not exit the loop
+// The loop has preheader
----------------
Nitpicks: 1) sentences should end with a dot. 2) I'd prefer to have some bullets in this list (I mean formatting), like:
```
- property1
- property2
```

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:207
+  const BranchInst *BI = dyn_cast<const BranchInst>(OrigH->getTerminator());
+  if (!BI || !BI->isConditional())
     return false;
----------------
Maybe I forgot something, but why exactly do we still need this check? I thought that one of the main benefits of this patch was to remove this limitation.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:261
+    LoopSize += Metrics.NumInsts;
+    // TODO: When the LoopSize is greater than RotationMaxSize, we can only
+    // rotate until the previous BB when LoopSize was less than RotationMaxSize.
----------------
Why is it called 'TODO'?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:280-281
+        int NLVal = PN->getBasicBlockIndex(NewLatch);
+        if (NLVal < 0)
+          continue;
+        int NPHVal = PN->getBasicBlockIndex(NewPreheader);
----------------
Is it possible at all? I think in this loop we expect phi nodes to be of a some specific form, i.e. having 2 operands: NewLatch and NewPreheader. Shouldn't we just assert this instead of `continue` the loop?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:357
+// (bounded by \p OrigH and \p OrigLatch), which are exiting the loop in
+// \p Blocks and collect all the exits from the region in \p Exits Returns the
+// first basic block which was not copied.
----------------
Missing dot before `Returns`.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:659
+
+  if (!isProfitableToRotate(Blocks, Exits))
+    return false;
----------------
Please rename the function per the previous discussion.

================
Comment at: llvm/test/Analysis/ScalarEvolution/2012-03-26-LoadConstant.ll:1
-; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -simplifycfg -S | FileCheck %s
+
----------------
mzolotukhin wrote:
> hiraditya wrote:
> > mzolotukhin wrote:
> > > Why is this change needed?
> > Now that the loop rotation does not simplify any instructions, we need simplifycfg post-licm to clean up the redundancies exposed by loop rotate.
> This test is no longer relevant, it doesn't fail if the change it went in with is reverted.
> 
> Could you please remove it (in a separate commit) altogether?
Still relevant.

================
Comment at: llvm/test/Transforms/LoopRotate/phi-duplicate.ll:36
 ; CHECK:      for.body:
-; CHECK-NEXT:   %j.01 = phi i64
-; CHECK-NOT:  br
-; CHECK:   br i1 %cmp, label %for.body, label %for.end
-; CHECK:      for.end:
-; CHECK-NEXT:        ret void
+; CHECK:  %phi.nh = phi i64
+; CHECK-NOT: = phi
----------------
It can be `CHECK-NEXT` I guess.

================
Comment at: llvm/test/Transforms/LoopRotate/preserve-loop-simplify.ll:19
 ; CHECK: [[INNER_PREROTATE_PREHEADER]]:
-; CHECK-NEXT: br i1 {{[^,]*}}, label %[[INNER_PREROTATE_PREHEADER_SPLIT_RETURN:[^,]*]], label %[[INNER_ROTATED_PREHEADER:[^,]*]]
+; CHECK: br label %inner.header.lr
+
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > hiraditya wrote:
> > > mzolotukhin wrote:
> > > > Why did this test change? Shouldn't we be producing the same results on such cases?
> > > Thanks for pointing this out. I have added code to restore the loop-simplify form.
> > Can't we cancel the changes to the test then?
> The names of copied basic blocks are not the same, and the order also changes because of extra basic blocks, so we need the changes here.
Could you please show what was the original behavior in this test and what do we do now? I'd like to make sure we are not rendering this test useless.

================
Comment at: llvm/test/Transforms/LoopVectorize/vect.omp.persistence.ll:7-11
 ; CHECK-NOT: LV: Loop hints: force=
-; In total only 1 loop should be rotated.
-; CHECK: 1 loop-rotate
+; checking the stats for the function @nonrotated
+; CHECK: 2 gvn{{.*}}Number of equalities propagated
+; With the new loop rotation both the loops should be rotated.
+; CHECK: 2 loop-rotate
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > mzolotukhin wrote:
> > > This test actually should probably be rewritten.
> > > 
> > > It tries to check that 
> > > 1) loop-rotate doesn't drop metadata,
> > > 2) no pass in -O2 pipeline before the vectorizer drops metadata.
> > > 
> > > The first check should be a separate test for loop-rotate, so I suggest adding it explicitly now (or in a separate commit). Then, one function from this test can be removed, and comments updates.
> > The test in the current form is hard to maintain, and with every change we risk that it becomes irrelevant. Could you please address it?
> I can clone this test in test/Transforms/LoopRotate directory and add the loop rotation related checks, if that is more reasonable.
Yes, we should do something like this. Please do this in a separate patch.

https://reviews.llvm.org/D22630