[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())
+  // For the purposes of hoisting instructions, inalloca allocas are treated as
+  // having side effects to avoid moving them across stacksave/stackrestore
+  // boundaries.
+  if (I->mayHaveSideEffects() ||
+      (isa<AllocaInst>(I) && cast<AllocaInst>(I)->isUsedWithInAlloca()))
     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:
   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