[PATCH] D22630: Loop rotation

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi,

Sorry for the long delay, I'm now trying to catch up with the reviews after the conference. Please find some comments below.

Thanks,
Michael



================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:548
+  DEBUG(DT->verifyDomTree());
+  assert(L->isRecursivelyLCSSAForm(*DT, *LI) && "Loop is not in LCSSA form.");
 
----------------
Could we also check that LoopSimplify is also preserved?


================
Comment at: llvm/test/Analysis/GlobalsModRef/memset-escape.ll:9-21
+; Check that load and the call to abort is redundant.
+; CHECK:   store i32 1, i32* getelementptr inbounds ([3 x i32], [3 x i32]* @a, i64 0, i64 2), align 4
+; CHECK:   store i32 0, i32* @b, align 4
+; CHECK:   br label %for.body
 
-; CHECK-LABEL: @main
-; CHECK: call void @llvm.memset.p0i8.i64{{.*}} @a
-; CHECK: store i32 3
-; CHECK: load i32, i32* getelementptr {{.*}} @a
-; CHECK: icmp eq i32
-; CHECK: br i1
+; CHECK: for.body:                                         ; preds = %for.body.preheader
+; CHECK:   store i32 0, i32* getelementptr inbounds ([3 x i32], [3 x i32]* @a, i64 0, i64 0), align 4
----------------
Can you please explain why this test was changed?


================
Comment at: llvm/test/Transforms/LoopRotate/alloca.ll:8-10
+; CHECK: alloca i8
+; CHECK: call void @use
+; CHECK: alloca i8
----------------
Do we really need this change?


================
Comment at: llvm/test/Transforms/LoopRotate/dbgvalue.ll:8-10
+; CHECK: tailrecurse.lr
+; CHECK: call void @llvm.dbg.value(metadata i32 %x
+; CHECK: tail call void @llvm.dbg.value(metadata i32 %y
----------------
Why was this test changed?


================
Comment at: llvm/test/Transforms/LoopRotate/indirectbr-1.ll:1
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck %s
+; ModuleID = 'ind.c'
----------------
Can we merge all tests with the same command lines to a single file?


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-before-latch.ll:6-7
+
+; CHECK-LABEL:foo
+; CHECK-NOT: loop-rotate
+
----------------
Please add a comment about what we check in this test.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:1-9
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK1 %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK2 %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK3 %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK4 %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK5 %s
+; RUN: opt -S < %s -loop-rotate -verify-dom-info -verify-loop-info | FileCheck -check-prefix=CHECK6 %s
----------------
Why can't we run them together?


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:15
+; Check that the loop is rotated.
+; CHECK-LABEL: bar0
+; CHECK: while.cond.lr:                                    ; preds = %entry
----------------
Better to use `@bar0` to avoid potential undesired matches somewhere else in the output.


================
Comment at: llvm/test/Transforms/LoopRotate/multiple-exits.ll:73-74
 ; CHECK: if.end:
-; CHECK: %inc = add i32 %i.02, 1
-; CHECK: %cmp = icmp eq i32 %inc, %x
-; CHECK: br i1 %cmp, label %for.cond.return.loopexit_crit_edge, label %for.body
+; CHECK: %inc = add i32 %phi.nh, 1
+; CHECK: %cmp = icmp eq i32 %i.0, %x
+; CHECK: br i1 %cmp, label %return.loopexit, label %for.body
----------------
Can we check the expected IR in more details? Currently, from the CHECK lines it's hard to tell what we expect (and it's hard to assess the change proposed in this patch). Comments also might help.


================
Comment at: llvm/test/Transforms/LoopRotate/nosimplifylatch.ll:6-7
 ;CHECK: for.inc:
-;CHECK-NEXT: %incdec.ptr.i = getelementptr 
+;CHECK: %incdec.ptr.i = getelementptr
+;CHECK: br
 
----------------
Why do we need this change?


================
Comment at: llvm/test/Transforms/LoopRotate/phi-duplicate.ll:31
 
 ; Should only end up with one phi.
 ; CHECK-LABEL:      define void @test(
----------------
bjope wrote:
> hiraditya wrote:
> > bjope wrote:
> > > The sole purpose with this test case seems to be to verify PR5837, and to make sure that there is only one phi (no duplicates created). Now you have introduced new CHECK statements to verify that we do get multple phi nodes.
> > > 
> > > If we still think that this test case is important and that that we only want want phi for this code, then I guess it still should verify that there is only one phi.
> > > If the world has changed and we now think that multiple phi nodes are wonderful, then I either expect that the test case is obsolete and can be removed. Or there should be a pretty good informative explanation why the test case has mutated in this way (no longer verifying the same thing as it initially was intended do verify).
> > Thanks for pointing this out. The current loop-rotation calls SSAUpdater::RewriteUse to remove multiple PHIs. The new loop rotation is simpler and does not use SSAUpdater. For this test case calling 
> > ```
> > ./bin/opt -loop-rotate -simplifycfg -S < ../llvm/test/Transforms/LoopRotate/phi-duplicate.ll
> > ```
> > would remove multiple phis (simplifycfg is called later in the pass pipeline at -O2 and -O3 anyways). I can edit the test case by adding -simplifycfg or modify the comment as well whichever you prefer.
> I have no personal preference here. I just happened to notice that the test case no longer mapped to the description/history of the test case (such as this comment, the reference to PR5837 earlier in this file, etc).
> 
> If there is no purpose with the test case (except showing the history of how loop-rotate has evovled), then maybe it should be removed? (with that said - I have no idea if it is custom to remove test cases in llvm)
> 
> Simplest solution is probably to just add some comments describing what the original purpose with the test case was, and why it has been changed into testing something else.
If these phi-nodes are unnecessary, I think we should not create them in the first place. Loop-rotate is usually one of the first passes among loop passes and other loop passes will deal with what loop-rotate produces. Simplifycfg will only be run afterwards.

How hard will it be to clean the phis up in loop-rotate?


================
Comment at: llvm/test/Transforms/LoopRotate/pr7447.ll:4-5
+
+; PR7447: there should be one loop rotated.
+; CHECK: 1 loop-rotate
+
----------------
Can we instead check the actual CFG and not rely on the debug/stats output?


================
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
+
----------------
Why did this test change? Shouldn't we be producing the same results on such cases?


================
Comment at: llvm/test/Transforms/LoopRotate/simplifylatch.ll:47
+;CHECK-LABEL: @foo(
+;CHECK: for.body.lr:
+;CHECK: %arrayidx.lr = getelementptr inbounds i8, i8* %CurPtr, i64 %idxprom.lr
----------------
Does rotated loop has a preheader now? If not, we're breaking LoopSimplify form, which is not good.


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


https://reviews.llvm.org/D22630