[PATCH] D22630: Loop rotation

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked an inline comment as done.
hiraditya added inline comments.


================
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);
+}
----------------
mzolotukhin wrote:
> Can we remove this function and use `SmallVector::append` instead?
We can but then there should be a way to get the Attachments from MDAttachmentMap. Currently there is none.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:207
+  const BranchInst *BI = dyn_cast<const BranchInst>(OrigH->getTerminator());
+  if (!BI || !BI->isConditional())
     return false;
----------------
mzolotukhin wrote:
> 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.
If the loop-header has invoke instruction, or some other TerminatorInst, I'm not sure how to deal with them.


================
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.
----------------
mzolotukhin wrote:
> Why is it called 'TODO'?
Because, even if the loop's size is greater than RotationMaxSize, what we can do is peel fewer basic  blocks than needed to completely rotate the loop. That way, at least, some redundancies will be exposed.


================
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:
> 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.
The test fails if I remove simplifycfg.


================
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
+
----------------
mzolotukhin wrote:
> 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.
Originally, only the basic block inner.body was cloned outside the loop which became the new loop header. In the new implementation inner.header and inner.body both are cloned outside the loop such that inner.latch becomes the new loop header.


https://reviews.llvm.org/D22630