[PATCH] D22630: Loop rotation

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin added inline comments.


================
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.
----------------
hiraditya wrote:
> 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.
I see, then I guess it should be expressed clearer in the comment. Currently it's not obvious what is there that needs to be done.


================
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
+
----------------
hiraditya wrote:
> 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.
It doesn't test what it was supposed to test. We either need to make sure it's useful, or remove it. I tried to revert the change the test was commited with and the test still passed, which means it no longer checks what it checked before. If you remove `-simplifycfg` it starts to fail, sure (I assume because there is `for.body` in the output) - but can you explain why there shouldn't be `for.body`? Can you explain what this test does? Juggling flags just to make it passes doesn't help us and only slows down everyone (mostly you, as you have to deal with it:) ).


================
Comment at: llvm/test/Transforms/LoopRotate/indirectbr-1.ll:6
+target triple = "x86_64-unknown-linux-gnu"
+; Check that the loop-latch with indirect branch is not rotated.
+; CHECK-NOT: {{.*}}.lr
----------------
We have a test checking this in `loop-rotate.ll` (`@foo3`). How are they different? Can we combine them (and place in one file)?


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:210
+
+; Check that the loop with pre-header and loop-body having a switch block is rotated.
+; CHECK-LABEL: @bar5
----------------
Will the following routine test the same?
```
define i32 @bar5() {

entry:
  br label %loop.header

loop.header:
  %a = phi i32 [ %b, %latch ], [ 0, %entry ]
  switch i8 undef, label %loop.body [
    i8 0, label %unreach
    i8 1, label %unreach
  ]

loop.body:
  br i1 undef, label %latch, label %return

latch:
  %b = phi i32 [ %a, %loop.body ]
  br label %loop.header

unreach:
  unreachable

return:
  ret i32 0
}
```
If it's sufficient, can you minimize other test in the similar manner? It should be much easier to understand what's going on, and hopefully they'll be much less fragile.


================
Comment at: llvm/test/Transforms/LoopRotate/multiple-exits-merge-phi.ll:2-3
+; RUN: opt -S -loop-rotate < %s -verify-loop-info -verify-dom-info | FileCheck %s
+; ModuleID = 'bugpoint-reduced-simplified.bc'
+source_filename = "bugpoint-output-fdebd3b.bc"
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
----------------
Please remove this.


================
Comment at: llvm/test/Transforms/LoopRotate/multiple-exits-merge-phi.ll:8
+; Function Attrs: nounwind readonly
+define void @test1() #0 {
+entry:
----------------
How is it different from the test in `multiple-exits.ll`? What phis are merged (the name of the test hints that they should be)?


================
Comment at: llvm/test/Transforms/LoopRotate/vect.omp.persistence.ll:1-2
+; RUN: opt < %s -O2 -force-vector-interleave=2 -force-vector-width=4 -debug-only=loop-vectorize -stats -S 2>&1 | FileCheck %s
+; RUN: opt < %s -loop-rotate -S | FileCheck -check-prefix=CHECK1 %s
+; REQUIRES: asserts
----------------
We don't need to run the entire pipeline to check that loop-rotate doesn't drop metadata. Please keep only the loop-rotate part here (but please make sure we actually check the metadata - that's what this test was about).


================
Comment at: llvm/test/Transforms/LoopVectorize/vect.omp.persistence.ll:19
 ; Ensure that "llvm.loop.vectorize.enable" metadata was not lost prior to LoopVectorize pass.
 ; In past LoopRotate was clearing that metadata.
 ;
----------------
Please update the comments, as the test has nothing to do with loop-rotate now. I'd recommend committing it separately.


https://reviews.llvm.org/D22630