[PATCH] D22630: Loop rotation

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked 3 inline comments as done.
hiraditya added inline comments.


================
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:
> > > 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:) ).
The for.body gets deleted because, once the loop is rotated everything in the loop becomes redundant/invariant. Previously, loop-rotate used to call simplifycfg, which I removed. So, I added the -simplifycfg in this test to make sure that the behavior is preserved.

I agree, it does not seem to test what it is supposed to. I can just remove this test (maybe in a separate patch together with changes related to vect.omp.persistence.ll)


================
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
----------------
mzolotukhin wrote:
> We have a test checking this in `loop-rotate.ll` (`@foo3`). How are they different? Can we combine them (and place in one file)?
This test checks that loop-latch having indirect branch is not rotated. This is different from all the tests in loop-rotate.ll. I'll merge this in loop-rotate.ll.


================
Comment at: llvm/test/Transforms/LoopRotate/multiple-exits-merge-phi.ll:8
+; Function Attrs: nounwind readonly
+define void @test1() #0 {
+entry:
----------------
mzolotukhin wrote:
> 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)?
It seems both are same. merge-phi is the phi in return statement, I was just trying to distinguish this from loop-header phi. I'll remove this test as this is redundant.


================
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.
 ;
----------------
mzolotukhin wrote:
> Please update the comments, as the test has nothing to do with loop-rotate now. I'd recommend committing it separately.
Will do


https://reviews.llvm.org/D22630