[PATCH] D22630: Loop rotation

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:489
+bool LoopRotate::preserveLoopSimplifyForm(const SmallPtrSetBB &Exits) const {
+    bool HasIndirectBr = false;
+    // Split the edge from exiting BB to exit BB if not in loop-simplify form.
----------------
mzolotukhin wrote:
> Wouldn't it be better to check if the loop has indirect branches and bail out before we begin doing anything?
This pass is able to rotate the loop when there are indirect branches inside it, so that would help expose redundancies in more cases. If there is a failing case or there is a regression please let me know.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:515
+/// Blocks[0] is the entry basic block of the SEME.
+/// TODO: arrange newly inserted bbs.
+void LoopRotate::rotateLoop(BasicBlock *NewH, const SmallVecBB &Blocks,
----------------
mzolotukhin wrote:
> We're doing something to arrange new blocks below. Please either say what specifically is missing or remove this TODO in case it's actually done.
I'll remove this.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:519
+  bool LoopSimplifyForm = false;
+  DEBUG(LoopSimplifyForm = L->isLoopSimplifyForm());
+  BasicBlock *OrigH = L->getHeader();
----------------
mzolotukhin wrote:
> Why is this under `DEBUG`?
This variable is only used for verification and as such not needed in release mode.


================
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:
> > > 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.


================
Comment at: llvm/test/Transforms/LoopRotate/simplifylatch.ll:48-60
+;CHECK: %arrayidx.lr = getelementptr inbounds i8, i8* %CurPtr, i64 %idxprom.lr
+;CHECK: %0 = load i8, i8* %arrayidx.lr, align 1
+;CHECK: %conv.lr = sext i8 %0 to i32
+;CHECK: %arrayidx1.lr = getelementptr inbounds i8, i8* %CurPtr, i64 0
+;CHECK: %1 = load i8, i8* %arrayidx1.lr, align 1
+;CHECK: %conv2.lr = sext i8 %1 to i32
+;CHECK: for.body:
----------------
mzolotukhin wrote:
> If we really want to check the entire contents of the loop, then we should use `CHECK-NEXT`. However, I'm not a fan of it - we should only check what matters.
Thanks for the suggestion. I removed the test because it wasn't checking anything special.


================
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
----------------
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.


https://reviews.llvm.org/D22630