[LoopUnroll] Handle certain PHIs in full unrolling properly.

[ebevhan]

Summary

When reconstructing the CFG of the loop after unrolling,
LoopUnroll could in some cases remove the phi operands of
loop-carried values instead of preserving them, resulting
in undef phi values after loop unrolling.

When doing this reconstruction, avoid removing incoming
phi values for phis in the successor blocks if the successor
is the block we are jumping to anyway.

[lebedev.ri]

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

8

passing-by remark: it's way too fragile to test exact value names,
you may want to test more of the output (capturing the value names elsewhere),
if not all of it.

[ebevhan]

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

8

I suppose I can generate the checks with the python script.

[lebedev.ri]

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

8

In this case i wasn't insisting on that - it gets pretty unreadable for large functions too.
But if it is autogenerated, you might as well precommit the original test.

<.. just passing by ..>

[fhahn]

Thanks for looking into this!

lib/Transforms/Utils/LoopUnroll.cpp

726

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.

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

39

Can you strip away the unnecessary stores/loads? They do not impact unrolling.

60

I think the test would be slightly better if we make %tmp2 loop dependent.

68

I think the attributes are not necessary.

[ebevhan]

lib/Transforms/Utils/LoopUnroll.cpp

726

I see, thanks for further insight! I'll make these changes.

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

39

What should I replace the sdiv with in that case? Just a single load?

[fhahn]

It would be great if you could add another function to the test, where we do not fully unroll the loop, i.e. by making sure it has a high trip count. For that, the minsize/optsize attributes need to be removed. Also, could you change the test file name to something a bit more descriptive, like unroll-header-exiting-with-phis.ll?

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

39

it's only used in %.lcssa10, you can replace it with an arbitrary constant there.

Instead of %tmp2 outside the loop, it would be good to have a loop dependent load or something, e.g. (simply pass an i16* %A as argument to the function).

%ptr = getelementptr inbounds i16, i16* %A, i64 %i.0
%tmp2 = load i16, i16* %ptr, align 1

48

Another nit: we could make %i.0 start at 0 and adjust the check in %cmp accordingly. Maybe also change it so we have more than 1 iteration to unroll, e.g. 2 or 3 iterations.

[fhahn]

LGTM, thanks! I think it would be good to also pick this for the 9.0 release.

[efriedma]

Yes, makes sense for 9.0

[fhahn]

@ebevhan it would be great if you could land this soon, so we can get it into the 9.0 release branch

[ebevhan]

@fhahn I don't have commit access so if someone could land it for me, that would be great!

[bjope]

In D66334#1644758, @ebevhan wrote:

@fhahn I don't have commit access so if someone could land it for me, that would be great!

I'll land this on trunk.
Any extra action needed from my side to get it into 9.0?

[fhahn]

In D66334#1644763, @bjope wrote:

In D66334#1644758, @ebevhan wrote:

@fhahn I don't have commit access so if someone could land it for me, that would be great!

I'll land this on trunk.
Any extra action needed from my side to get it into 9.0?

Thanks! Once the release branch is created, you have to file a bug to get a commit merged into the release branch, which I did for this patch: https://bugs.llvm.org/show_bug.cgi?id=43118

[bjope]

In D66334#1645356, @fhahn wrote:

In D66334#1644763, @bjope wrote:

In D66334#1644758, @ebevhan wrote:

@fhahn I don't have commit access so if someone could land it for me, that would be great!

I'll land this on trunk.
Any extra action needed from my side to get it into 9.0?

Thanks! Once the release branch is created, you have to file a bug to get a commit merged into the release branch, which I did for this patch: https://bugs.llvm.org/show_bug.cgi?id=43118

In D66334#1645356, @fhahn wrote:

In D66334#1644763, @bjope wrote:

In D66334#1644758, @ebevhan wrote:

@fhahn I don't have commit access so if someone could land it for me, that would be great!

I'll land this on trunk.
Any extra action needed from my side to get it into 9.0?

Thanks! Once the release branch is created, you have to file a bug to get a commit merged into the release branch, which I did for this patch: https://bugs.llvm.org/show_bug.cgi?id=43118

Great. I'll use that bug report as a template the next time :-)

lib/Transforms/Utils/LoopUnroll.cpp

if (NeedConditional) {
       // Update the conditional branch's successor for the following
       // iteration.
       Term->setSuccessor(!ContinueOnTrue, Dest);
     } else {
       // Remove phi operands at this loop exit
       if (Dest != LoopExit) {
         BasicBlock *BB = Src;
         for (BasicBlock *Succ : successors(BB)) {
-          if (Succ == CurrentHeader)
+          if (Succ == CurrentHeader || Succ == Dest)
             continue;
           for (PHINode &Phi : Succ->phis())
             Phi.removeIncomingValue(BB, false);
         }
       }
       // Replace the conditional branch with an unconditional one.
       BranchInst::Create(Dest, Term);
       Term->eraseFromParent();

test/Transforms/LoopUnroll/full-unroll-miscompile.ll

+; RUN: opt < %s -S -loop-unroll | FileCheck %s
+
+; The phi which acts as input to func should not be undef. It should
+; have its loop-carried value (the load of d in the entry block)
+; replaced accordingly after unrolling the loop.
+
+; CHECK-LABEL: main
+; CHECK: %.lcssa10.lcssa = phi i16 [ %tmp2, %for.cond.cleanup3 ]
+
+@a = dso_local global i16 0, align 1
+@b = dso_local global i16 0, align 1
+@c = dso_local global [2 x i16] [i16 1, i16 1], align 1
+@d = dso_local global i16 0, align 1
+@e = dso_local global i64 0, align 1
+
+; Function Attrs: minsize nounwind optsize
+define dso_local i16 @main() #0 {
+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
+  %div = sdiv i16 %tmp, %tmp1
+  store i16 %div, i16* @b, align 1
+  %tmp2 = load i16, i16* @d, align 1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond.cleanup3, %entry
+  %.lcssa10 = phi i16 [ %div, %entry ], [ %.lcssa, %for.cond.cleanup3 ]
+  %i.0 = phi i64 [ 37, %entry ], [ %inc9, %for.cond.cleanup3 ]
+  %cmp = icmp ult i64 %i.0, 38
+  br i1 %cmp, label %for.cond.cleanup3, label %for.cond.cleanup
+
+for.cond.cleanup:                                 ; preds = %for.cond
+  %.lcssa10.lcssa = phi i16 [ %.lcssa10, %for.cond ]
+  store i16 %.lcssa10.lcssa, i16* @b, align 1
+  store i64 1, i64* @e, align 1
+  %tmp3 = call i16 (i16) @func(i16 %.lcssa10.lcssa)
+  ret i16 0
+
+for.cond.cleanup3:                                ; preds = %for.cond
+  %.lcssa = phi i16 [ %tmp2, %for.cond ]
+  %inc9 = add i64 %i.0, 1
+  br label %for.cond
+}
+
+; Function Attrs: nounwind
+declare i16 @func(i16) #1
+
+attributes #0 = { minsize nounwind optsize }
+attributes #1 = { nounwind }