[PATCH] D22630: Loop rotation

[Bjorn Pettersson via llvm-commits]

bjope 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(
----------------
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.


https://reviews.llvm.org/D22630