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

[Eli Friedman]

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