[SimplifyCFG] Don't hoist allocas

[aeubanks]

Summary

D129370 started hoisting allocas across stacksave/stackrestore
boundaries which is wrong.

[aeubanks]

remove FIXME

[efriedma]

Why specifically "inalloca" allocas? Don't all allocas have the same issue?

[aeubanks]

all allocas

[chill]

Thanks! LGTM.

There's a comment about mayHaveSideEffects that's not entirely correct, maybe it can be reworded somewhat?
https://github.com/llvm/llvm-project/blob/1f7b94e0ec7fae446e2ef9fcc41968fc711fb2c7/llvm/include/llvm/IR/Instruction.h#L630

[rnk]

In D133730#3785909, @chill wrote:

There's a comment about mayHaveSideEffects that's not entirely correct, maybe it can be reworded somewhat?
https://github.com/llvm/llvm-project/blob/1f7b94e0ec7fae446e2ef9fcc41968fc711fb2c7/llvm/include/llvm/IR/Instruction.h#L630

The comment suggests isSafeToSpeculativelyExecute. Perhaps that is more appropriate for this transform, which hoists? Either way, correctness first, let's land the fix and you can iterate on improvements and simplifications.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

enum SkipFlags {
   SkipSideEffect = 2,
   SkipImplicitControlFlow = 4
 };
 
 static unsigned skippedInstrFlags(Instruction *I) {
   unsigned Flags = 0;
   if (I->mayReadFromMemory())
     Flags |= SkipReadMem;
-  if (I->mayHaveSideEffects())
+  // We can't arbitrarily move around allocas, e.g. moving allocas (especially
+  // inalloca) across stacksave/stackrestore boundaries.
+  if (I->mayHaveSideEffects() || isa<AllocaInst>(I))
     Flags |= SkipSideEffect;
   if (!isGuaranteedToTransferExecutionToSuccessor(I))
     Flags |= SkipImplicitControlFlow;
   return Flags;
 }
 
 // Returns true if it is safe to reorder an instruction across preceding
 // instructions in a basic block.

llvm/test/Transforms/SimplifyCFG/hoist-common-skip.ll

if.end:
   %u = phi i16 [ %c.0, %if.then ], [ %c.1, %if.else ]
 
   %w = add i16 %v, %u
 
   ret i16 %w
 }
 
 ;; Don't hoist stacksaves across inalloca allocas
-;; FIXME: currently this is miscompiled
 define void @f10(i1 %cond) {
 ; CHECK-LABEL: @f10(
 ; CHECK-NEXT:    [[SS:%.*]] = call ptr @llvm.stacksave()
-; CHECK-NEXT:    [[SS2:%.*]] = call ptr @llvm.stacksave()
-; CHECK-NEXT:    [[I2:%.*]] = alloca inalloca i64, align 8
 ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
 ; CHECK:       bb1:
 ; CHECK-NEXT:    [[I1:%.*]] = alloca inalloca i32, align 4
+; CHECK-NEXT:    [[SS2:%.*]] = call ptr @llvm.stacksave()
+; CHECK-NEXT:    [[I2:%.*]] = alloca inalloca i64, align 8
 ; CHECK-NEXT:    call void @inalloca_i64(ptr inalloca(i64) [[I2]])
 ; CHECK-NEXT:    call void @llvm.stackrestore(ptr [[SS2]])
 ; CHECK-NEXT:    call void @inalloca_i32(ptr inalloca(i32) [[I1]])
 ; CHECK-NEXT:    br label [[END:%.*]]
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[I3:%.*]] = alloca inalloca i64, align 8
-; CHECK-NEXT:    [[TMP1:%.*]] = call ptr @inalloca_i64(ptr inalloca(i64) [[I2]])
-; CHECK-NEXT:    call void @llvm.stackrestore(ptr [[SS2]])
+; CHECK-NEXT:    [[SS3:%.*]] = call ptr @llvm.stacksave()
+; CHECK-NEXT:    [[I4:%.*]] = alloca inalloca i64, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = call ptr @inalloca_i64(ptr inalloca(i64) [[I4]])
+; CHECK-NEXT:    call void @llvm.stackrestore(ptr [[SS3]])
 ; CHECK-NEXT:    [[TMP2:%.*]] = call ptr @inalloca_i64(ptr inalloca(i64) [[I3]])
 ; CHECK-NEXT:    br label [[END]]
 ; CHECK:       end:
 ; CHECK-NEXT:    call void @llvm.stackrestore(ptr [[SS]])
 ; CHECK-NEXT:    ret void
 ;
   %ss = call ptr @llvm.stacksave()
   br i1 %cond, label %bb1, label %bb2