[PATCH] D66334: [LoopUnroll] Handle certain PHIs in full unrolling properly.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for looking into this!



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:726
         for (BasicBlock *Succ : successors(BB)) {
-          if (Succ == CurrentHeader)
+          if (Succ == CurrentHeader || Succ == Dest)
             continue;
----------------
Currently we have a problem when unrolling loops which exit via the header. We should only preserve the incoming value from BB for the block in the current loop we jump to. For loops where we exit via the latch, this is the current header of the loop. For loops exiting via the header, this should be the successor of the header in the loop.


 I think it would be slightly better to just change the name of CurrentHeader and then pass HeaderSucc[I] at line 797, to avoid unnecessary checks and keep the code slightly simpler. Maybe also add a comment in setDest to explain better what is going on.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-miscompile.ll:38
+entry:
+  %tmp = load i16, i16* @a, align 1
+  %tmp1 = load i16, i16* getelementptr inbounds ([2 x i16], [2 x i16]* @c, i32 0, i32 0), align 1
----------------
Can you strip away the unnecessary stores/loads? They do not impact unrolling.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-miscompile.ll:59
+for.cond.cleanup3:                                ; preds = %for.cond
+  %.lcssa = phi i16 [ %tmp2, %for.cond ]
+  %inc9 = add i64 %i.0, 1
----------------
I think the test would be slightly better if we make %tmp2 loop dependent.


================
Comment at: test/Transforms/LoopUnroll/full-unroll-miscompile.ll:67
+
+attributes #0 = { minsize nounwind optsize }
+attributes #1 = { nounwind }
----------------
I think the attributes are not necessary.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66334/new/

https://reviews.llvm.org/D66334