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

Mon Aug 19 02:06:41 PDT 2019

ebevhan marked 2 inline comments as done.
ebevhan added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:726
         for (BasicBlock *Succ : successors(BB)) {
-          if (Succ == CurrentHeader)
+          if (Succ == CurrentHeader || Succ == Dest)
             continue;
----------------
fhahn wrote:
> 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.
I see, thanks for further insight! I'll make these changes.

================
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
----------------
fhahn wrote:
> Can you strip away the unnecessary stores/loads? They do not impact unrolling.
What should I replace the sdiv with in that case? Just a single load?

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

https://reviews.llvm.org/D66334