Issues with the llvm.stackrestore intrinsic

Hi,

I have two problems regarding the llvm.stackrestore intrinsic. I'm
running on 3.0, but a quick test on trunk also showed the same behavior.

First problem:

Hello Patrik,

In Intrinsics.td it says

// Note: we treat stacksave/stackrestore as writemem because we don't
otherwise
// model their dependencies on allocas.
def int_stacksave : Intrinsic<[llvm_ptr_ty]>,
GCCBuiltin<"__builtin_stack_save">;
def int_stackrestore : Intrinsic<[], [llvm_ptr_ty]>,
GCCBuiltin<"__builtin_stack_restore">;

Does "GCCBuiltin" imply "writemem", or how does the comment and the code
correspond?

"writemem" is by default. Look into the beginning of this .td file for
description of various flags.

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.

Patrik Hägglund wrote 2012-02-01 12:11:

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

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