[LLVMdev] [PATCH] Issues with the llvm.stackrestore intrinsic - now LoopRotation handling of alloca

[Patrik Hägglund H]

Is the following patch good enough?

Sorry, for the delay. I now see that the testcase below don't trigger the bug anymore (because of r150439?). But I don't know if I'm able to make a new one. I don't have my original (much larger) testcase available anymore, and I don't know much about theese transformations.

/Patrik Hägglund

diff --git a/lib/Transforms/Scalar/LoopRotation.cpp b/lib/Transforms/Scalar/LoopRotation.cpp
index 9fd0958..b85f189 100644
--- a/lib/Transforms/Scalar/LoopRotation.cpp
+++ b/lib/Transforms/Scalar/LoopRotation.cpp
@@ -236,7 +236,8 @@ bool LoopRotate::rotateLoop(Loop *L) {
     // memory (without proving that the loop doesn't write).
     if (L->hasLoopInvariantOperands(Inst) &&
         !Inst->mayReadFromMemory() && !Inst->mayWriteToMemory() &&
-        !isa<TerminatorInst>(Inst) && !isa<DbgInfoIntrinsic>(Inst)) {
+        !isa<TerminatorInst>(Inst) && !isa<DbgInfoIntrinsic>(Inst) &&
+        !isa<AllocaInst>(Inst)) {
       Inst->moveBefore(LoopEntryBranch);
       continue;
     }
diff --git a/test/Transforms/LoopRotate/alloca.ll b/test/Transforms/LoopRotate/alloca.ll
new file mode 100644
index 0000000..67dae4c
--- /dev/null
+++ b/test/Transforms/LoopRotate/alloca.ll
@@ -0,0 +1,33 @@
+; RUN: opt < %s -loop-rotate -S | FileCheck %s
+
+; Test alloca in -loop-rotate.
+
+; We expect a different value for %ptr each iteration (according to the
+; definition of alloca). I.e. each @llvm.use.ptr must be paired with an alloca.
+
+; CHECK: %ptr = alloca i8
+; CHECK-NEXT: call void @llvm.use.ptr(i8* %ptr)
+
+ at e = global i16 10
+
+declare void @llvm.use.ptr(i8*)
+
+define void @test() {
+entry:
+  %end = load i16* @e
+  br label %loop
+
+loop:
+  %n.phi = phi i16 [ %n, %loop.fin ], [ 0, %entry ]
+  %ptr = alloca i8
+  call void @llvm.use.ptr(i8* %ptr)
+  %cond = icmp eq i16 %n.phi, %end
+  br i1 %cond, label %exit, label %loop.fin
+
+loop.fin:
+  %n = add i16 %n.phi, 1
+  br label %loop
+
+exit:
+  ret void
+}

-----Original Message-----
From: Eli Friedman [mailto:eli.friedman at gmail.com] 
Sent: den 3 februari 2012 10:58
To: Patrik Hägglund H
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] Issues with the llvm.stackrestore intrinsic - now LoopRotation handling of alloca

2012/2/3 Patrik Hägglund <patrik.h.hagglund at ericsson.com>:
> Hi,
>
> I've tracked the first problem (mentioned in my previous email, quoted
> below) down further, ending up in the handling of alloca in
> LoopRotation.cpp (from trunk):
>
>      // If the instruction's operands are invariant and it doesn't read
> or write
>      // memory, then it is safe to hoist.  Doing this doesn't change the
> order of
>      // execution in the preheader, but does prevent the instruction from
>      // executing in each iteration of the loop.  This means it is safe
> to hoist
>      // something that might trap, but isn't safe to hoist something
> that reads
>      // memory (without proving that the loop doesn't write).
>      if (L->hasLoopInvariantOperands(Inst) &&
>          !Inst->mayReadFromMemory() && !Inst->mayWriteToMemory() &&
>          !isa<TerminatorInst>(Inst) && !isa<DbgInfoIntrinsic>(Inst)) {
>        Inst->moveBefore(LoopEntryBranch);
>        continue;
>      }
>
> The above code happily moves an alloca instruction out of the loop, to
> the new loop header. Shouldn't we also check on
>
>   !isa<AllocaInst>(Inst)
>
> before allowing to move it?
>
>
> The code that breaks because of the moved alloca looks like
>
> start:
>   [...]
>   br loopstart
>
> loopbody:
>   [use alloc'd mem]
>   call stackrestore(%oldsp)
>   br loopstart
>
> loopstart:
>    %oldsp = call stacksave()
>   alloca
>   [...]
>   brcond loopbody, end
>
> end:
>   [use alloc'd mem]
>   call stackrestore(%oldsp)
>
> Then LoopRotation clones the stacksave from bb3 to bb1 and _moves_ the
> alloca from bb3 to bb1. So the alloca is moved outside the loop instead
> of having it execute each lap. But there still is a stackrestore inside
> the loop that will "deallocate" the memory, so all but the first lap of
> the loop will access deallocated memory.
>
> So, shouldn't alloca instructions be cloned rather than moved? And maybe
> there are other instructions with side effects that also should be cloned.

I haven't read your description very carefully, but your analysis
looks roughly correct; we've had similar issues with
stacksave/stackrestore before.  See r143093, for example.

-Eli