[PATCH] D22630: Loop rotation

[Aditya Kumar via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/test/Transforms/LoopRotate/phi-duplicate.ll:31
 
 ; Should only end up with one phi.
 ; CHECK-LABEL:      define void @test(
----------------
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.


https://reviews.llvm.org/D22630