[SROA] Fix for PR23737: Handle Atomic when rewriting Loads and Stores

[philip.pfaffe]

Summary

SROA currently ignores loads and stores being atomic. When rewriting instructions, copy the atomic property to the new instructions.

Concerning integer- or vector-promotion, treat atomic like volatile and don't promote.

[majnemer]

This looks reasonable to me but I'd like to know how Chandler feels about this.

[chandlerc]

I don't think SROA is incorrect here. I'll comment on the bug, and we can sort out whether there is a bug here or not.

However, even if there is a bug, we shouldn't turn *off* SROA due to atomics, we should just preserve atomicity when necessary.

Specifically, if we can promote an alloca to an SSA variable, then only this thread references it, and so no synchronization could be occuring through the atomic accesses.

We can (and should) add instcombine patterns to strip these kinds of atomic flags as well, but we often want SROA to work even without instcombine having run in order to minimize the runs necessary in the inliner/sroa iteration sequence.

[philip.pfaffe]

This has been superseded by rL242126.

lib/Transforms/Scalar/SROA.cpp

static bool isVectorPromotionViableForSlice(AllocaSlices::Partition &P,
   } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser())) {
     if (II->getIntrinsicID() != Intrinsic::lifetime_start &&
         II->getIntrinsicID() != Intrinsic::lifetime_end)
       return false;
   } else if (U->get()->getType()->getPointerElementType()->isStructTy()) {
     // Disable vector promotion when there are loads or stores of an FCA.
     return false;
   } else if (LoadInst *LI = dyn_cast<LoadInst>(U->getUser())) {
-    if (LI->isVolatile())
+    if (!LI->isSimple())
       return false;
     Type *LTy = LI->getType();
     if (P.beginOffset() > S.beginOffset() || P.endOffset() < S.endOffset()) {
       assert(LTy->isIntegerTy());
       LTy = SplitIntTy;
     }
     if (!canConvertValue(DL, SliceTy, LTy))
       return false;
   } else if (StoreInst *SI = dyn_cast<StoreInst>(U->getUser())) {
-    if (SI->isVolatile())
+    if (!SI->isSimple())
       return false;
     Type *STy = SI->getValueOperand()->getType();
     if (P.beginOffset() > S.beginOffset() || P.endOffset() < S.endOffset()) {
       assert(STy->isIntegerTy());
       STy = SplitIntTy;
     }
     if (!canConvertValue(DL, STy, SliceTy))
       return false;
static bool isIntegerWideningViableForSlice(const Slice &S,
   // We can't reasonably handle cases where the load or store extends past
   // the end of the aloca's type and into its padding.
   if (RelEnd > Size)
     return false;
 
   Use *U = S.getUse();
 
   if (LoadInst *LI = dyn_cast<LoadInst>(U->getUser())) {
-    if (LI->isVolatile())
+    if (!LI->isSimple())
       return false;
     // Note that we don't count vector loads or stores as whole-alloca
     // operations which enable integer widening because we would prefer to use
     // vector widening instead.
     if (!isa<VectorType>(LI->getType()) && RelBegin == 0 && RelEnd == Size)
       WholeAllocaOp = true;
     if (IntegerType *ITy = dyn_cast<IntegerType>(LI->getType())) {
       if (ITy->getBitWidth() < DL.getTypeStoreSizeInBits(ITy))
         return false;
     } else if (RelBegin != 0 || RelEnd != Size ||
                !canConvertValue(DL, AllocaTy, LI->getType())) {
       // Non-integer loads need to be convertible from the alloca type so that
       // they are promotable.
       return false;
     }
   } else if (StoreInst *SI = dyn_cast<StoreInst>(U->getUser())) {
     Type *ValueTy = SI->getValueOperand()->getType();
-    if (SI->isVolatile())
+    if (!SI->isSimple())
       return false;
     // Note that we don't count vector loads or stores as whole-alloca
     // operations which enable integer widening because we would prefer to use
     // vector widening instead.
     if (!isa<VectorType>(ValueTy) && RelBegin == 0 && RelEnd == Size)
       WholeAllocaOp = true;
     if (IntegerType *ITy = dyn_cast<IntegerType>(ValueTy)) {
       if (ITy->getBitWidth() < DL.getTypeStoreSizeInBits(ITy))
Value *rewriteVectorizedLoadInst() {
     assert(EndIndex > BeginIndex && "Empty vector!");
 
     Value *V = IRB.CreateAlignedLoad(&NewAI, NewAI.getAlignment(), "load");
     return extractVector(IRB, V, BeginIndex, EndIndex, "vec");
   }
 
   Value *rewriteIntegerLoad(LoadInst &LI) {
     assert(IntTy && "We cannot insert an integer to the alloca");
-    assert(!LI.isVolatile());
+    assert(LI.isSimple());
     Value *V = IRB.CreateAlignedLoad(&NewAI, NewAI.getAlignment(), "load");
     V = convertValue(DL, IRB, V, IntTy);
     assert(NewBeginOffset >= NewAllocaBeginOffset && "Out of bounds offset");
     uint64_t Offset = NewBeginOffset - NewAllocaBeginOffset;
     if (Offset > 0 || NewEndOffset < NewAllocaEndOffset)
       V = extractInteger(DL, IRB, V, cast<IntegerType>(LI.getType()), Offset,
                          "extract");
     return V;
   }
 
   bool visitLoadInst(LoadInst &LI) {
     DEBUG(dbgs() << "    original: " << LI << "\n");
     Value *OldOp = LI.getOperand(0);
     assert(OldOp == OldPtr);
 
     Type *TargetTy = IsSplit ? Type::getIntNTy(LI.getContext(), SliceSize * 8)
                              : LI.getType();
     bool IsPtrAdjusted = false;
     Value *V;
     if (VecTy) {
       V = rewriteVectorizedLoadInst();
     } else if (IntTy && LI.getType()->isIntegerTy()) {
       V = rewriteIntegerLoad(LI);
-    } else if (NewBeginOffset == NewAllocaBeginOffset &&
-               canConvertValue(DL, NewAllocaTy, LI.getType())) {
+    } else {
+      if (NewBeginOffset == NewAllocaBeginOffset &&
         V = IRB.CreateAlignedLoad(&NewAI, NewAI.getAlignment(), LI.isVolatile(),
                                   LI.getName());
       } else {
         Type *LTy = TargetTy->getPointerTo();
         V = IRB.CreateAlignedLoad(getNewAllocaSlicePtr(IRB, LTy),
                                   getSliceAlign(TargetTy), LI.isVolatile(),
                                   LI.getName());
         IsPtrAdjusted = true;
       }
+      if (LI.isAtomic()) {
+        auto *NewLI = cast<LoadInst>(V);
+        if (!NewLI->getAlignment()) {
+          auto *LTy =
+              NewLI->getPointerOperand()->getType()->getPointerElementType();
+          NewLI->setAlignment(DL.getABITypeAlignment(LTy));
+        }
+        NewLI->setAtomic(LI.getOrdering(), LI.getSynchScope());
+      }
+    }
     V = convertValue(DL, IRB, V, TargetTy);
 
     if (IsSplit) {
       assert(!LI.isVolatile());
       assert(LI.getType()->isIntegerTy() &&
              "Only integer type loads and stores are split");
       assert(SliceSize < DL.getTypeStoreSize(LI.getType()) &&
              "Split load isn't smaller than original load");
if (IsSplit) {
       delete Placeholder;
     } else {
       LI.replaceAllUsesWith(V);
     }
 
     Pass.DeadInsts.insert(&LI);
     deleteIfTriviallyDead(OldOp);
     DEBUG(dbgs() << "          to: " << *V << "\n");
-    return !LI.isVolatile() && !IsPtrAdjusted;
+    return LI.isSimple() && !IsPtrAdjusted;
   }
 
   bool rewriteVectorizedStoreInst(Value *V, StoreInst &SI, Value *OldOp) {
     if (V->getType() != VecTy) {
       unsigned BeginIndex = getIndex(NewBeginOffset);
       unsigned EndIndex = getIndex(NewEndOffset);
       assert(EndIndex > BeginIndex && "Empty vector!");
       unsigned NumElements = EndIndex - BeginIndex;
bool rewriteVectorizedStoreInst(Value *V, StoreInst &SI, Value *OldOp) {
 
     (void)Store;
     DEBUG(dbgs() << "          to: " << *Store << "\n");
     return true;
   }
 
   bool rewriteIntegerStore(Value *V, StoreInst &SI) {
     assert(IntTy && "We cannot extract an integer from the alloca");
-    assert(!SI.isVolatile());
+    assert(SI.isSimple());
     if (DL.getTypeSizeInBits(V->getType()) != IntTy->getBitWidth()) {
       Value *Old =
           IRB.CreateAlignedLoad(&NewAI, NewAI.getAlignment(), "oldload");
       Old = convertValue(DL, IRB, Old, IntTy);
       assert(BeginOffset >= NewAllocaBeginOffset && "Out of bounds offset");
       uint64_t Offset = BeginOffset - NewAllocaBeginOffset;
       V = insertInteger(DL, IRB, Old, SI.getValueOperand(), Offset, "insert");
     }
if (NewBeginOffset == NewAllocaBeginOffset &&
       V = convertValue(DL, IRB, V, NewAllocaTy);
       NewSI = IRB.CreateAlignedStore(V, &NewAI, NewAI.getAlignment(),
                                      SI.isVolatile());
     } else {
       Value *NewPtr = getNewAllocaSlicePtr(IRB, V->getType()->getPointerTo());
       NewSI = IRB.CreateAlignedStore(V, NewPtr, getSliceAlign(V->getType()),
                                      SI.isVolatile());
     }
-    (void)NewSI;
+    if (SI.isAtomic()) {
+      if (!NewSI->getAlignment()) {
+        auto *STy =
+            NewSI->getPointerOperand()->getType()->getPointerElementType();
+        NewSI->setAlignment(DL.getABITypeAlignment(STy));
+      }
+      NewSI->setAtomic(SI.getOrdering(), SI.getSynchScope());
+    }
     Pass.DeadInsts.insert(&SI);
     deleteIfTriviallyDead(OldOp);
 
     DEBUG(dbgs() << "          to: " << *NewSI << "\n");
-    return NewSI->getPointerOperand() == &NewAI && !SI.isVolatile();
+    return NewSI->getPointerOperand() == &NewAI && SI.isSimple();
   }
 
   /// \brief Compute an integer value from splatting an i8 across the given
   /// number of bytes.
   ///
   /// Note that this routine assumes an i8 is a byte. If that isn't true, don't
   /// call this routine.
   /// FIXME: Heed the advice above.

test/Transforms/SROA/atomic.ll

+; RUN: opt -sroa -S < %s | FileCheck %s
+target triple = "x86_64-pc-linux"
+
+define void @PR23737(i64** %x) {
+; CHECK-LABEL: PR23737
+entry:
+  %ptr = alloca i64, align 8
+  store atomic i64 0, i64* %ptr seq_cst, align 8
+  %load = load atomic volatile i64, i64* %ptr seq_cst, align 8
+  ;CHECK: load atomic volatile i64, i64* %{{.*}} seq_cst, align 8
+  ret void
+}
+
+define void @test_align(i64** %x) {
+; CHECK-LABEL: test_align
+entry:
+  %ptr = alloca i64, align 4
+  store atomic i64 0, i64* %ptr seq_cst, align 4
+  ; CHECK: store atomic i64 0, i64* %{{.*}} seq_cst, align 4
+  %load = load atomic i64, i64* %ptr seq_cst, align 4
+  ; CHECK: load atomic i64, i64* %{{.*}} seq_cst, align 4
+
+  ret void
+}
+
+define i32 @test_vector_prom(<4 x i32> %x, <4 x i32> %y) {
+; CHECK-LABEL: @test_vector_prom
+entry:
+  %a = alloca [2 x <4 x i32>], align 8
+; CHECK: alloca
+
+  %a.tmp1 = getelementptr inbounds [2 x <4 x i32>], [2 x <4 x i32>]* %a, i64 0, i64 0, i64 2
+  %tmp1 = load atomic i32, i32* %a.tmp1 seq_cst, align 8
+; CHECK: load atomic
+  ret i32 %tmp1
+}
+
+%list = type { %list*, i32 }
+define i32 @test_integer_prom(i32 %i) {
+; CHECK-LABEL: @test_integer_prom
+entry:
+  %a = alloca %list, align 4
+  ; CHECK: alloca
+  %ptr = getelementptr inbounds %list, %list* %a, i32 0, i32 1
+  store atomic volatile i32 %i, i32* %ptr seq_cst, align 4
+; CHECK: store atomic volatile i32 %{{.*}}, i32* %{{.*}} seq_cst, align 4
+  ret i32 %i
+}