SROA: Don't drop atomic load/store alignments (PR45010)

[hans]

Summary

SROA will drop the explicit alignment on allocas when the ABI guarantees enough alignment. Because the alignment on new load/store instructions are set base on the alloca's alignment, that means SROA would end up dropping the alignment from atomic loads and stores, which is not allowed (see bug). For those, make sure to always carry over the alignment from the previous instruction.

[gchatelet]

Thank you for taking a look @hans !
Only two nits on my side.

llvm/lib/Transforms/Scalar/SROA.cpp

2523

NewLI->setAlignment(LI.getAlign());

2715

NewSI->setAlignment(SI.getAlign());

[hans]

Using getAlign().

[efriedma]

I would prefer to just avoid creating unaligned allocas in the first place. https://github.com/llvm/llvm-project/blob/740ed617f7d4d16e7883636c5eff994f8be7eba4/llvm/lib/Transforms/Scalar/SROA.cpp#L4163 has some code which intentionally uses an unspecified alignment... but really, there's no reason to do that. allocas with no specified alignment are implicitly aligned using the ABI type alignment. So it's semantically equivalent to std::max(Alignment, DL.getABITypeAlignment(SliceTy), just more annoying to deal with.

Really, the alignment of load/store/alloca shouldn't be a MaybeAlign in the first place, but someone needs to go through and clean up a bunch of places to make that happen.

[bkramer]

makes sense to me

[hans]

In D75266#1895826, @efriedma wrote:

I would prefer to just avoid creating unaligned allocas in the first place. https://github.com/llvm/llvm-project/blob/740ed617f7d4d16e7883636c5eff994f8be7eba4/llvm/lib/Transforms/Scalar/SROA.cpp#L4163 has some code which intentionally uses an unspecified alignment... but really, there's no reason to do that. allocas with no specified alignment are implicitly aligned using the ABI type alignment. So it's semantically equivalent to std::max(Alignment, DL.getABITypeAlignment(SliceTy), just more annoying to deal with.

The current behaviour is from https://github.com/llvm/llvm-project/commit/903790eff54f473fca8fbf6915713751a070b5df

Changing that seems like a riskier change (I want this for 10.x) whereas copying over the alignment from the store/load seems straight-forward to me.

[jfb]

llvm/test/Transforms/SROA/alignment.ll

242

From the code, it looks like you want a test where the load is volatile only as well?
Should probably test stores too?

Does this affect other memory operations, i.e. the atomic ones?

[efriedma]

Changing that seems like a riskier change (I want this for 10.x) whereas copying over the alignment from the store/load seems straight-forward to me.

It's not completely obvious at first glance that the alloca is actually sufficiently aligned. But looking a little more carefully, it should be; we're preserving the alignment of the original alloca. So this is fine, I guess. LGTM

[gchatelet]

Please address @jfb's comment but otherwise LGTM.

[hans]

llvm/test/Transforms/SROA/alignment.ll

242

I don't think the volatile-only case is interesting. My code doesn't change anything for that case.

I've made the test cover stores too.

I'm not sure what you mean by "other memory operations, i.e. the atomic ones". This change affects exactly the atomic memory operations..

[jfb]

I meant compare and exchange, as well as RMW.

[hans]

In D75266#1898238, @jfb wrote:

I meant compare and exchange, as well as RMW.

It doesn't look like SROA handles cmpxchg or atomicrmw instructions.

[jfb]

In D75266#1900542, @hans wrote:

In D75266#1898238, @jfb wrote:

I meant compare and exchange, as well as RMW.

It doesn't look like SROA handles cmpxchg or atomicrmw instructions.

I know, but if it grows that capability then the author will thank you for adding a test that prevents them from regressing correctness.

llvm/lib/Transforms/Scalar/SROA.cpp

if (VecTy) {
                  TargetTy->isIntegerTy()))) {
       LoadInst *NewLI = IRB.CreateAlignedLoad(NewAI.getAllocatedType(), &NewAI,
                                               NewAI.getAlign(), LI.isVolatile(),
                                               LI.getName());
       if (AATags)
         NewLI->setAAMetadata(AATags);
       if (LI.isVolatile())
         NewLI->setAtomic(LI.getOrdering(), LI.getSyncScopeID());
+      if (NewLI->isAtomic())
+        NewLI->setAlignment(LI.getAlign());
 
       // Any !nonnull metadata or !range metadata on the old load is also valid
       // on the new load. This is even true in some cases even when the loads
       // are different types, for example by mapping !nonnull metadata to
       // !range metadata by modeling the null pointer constant converted to the
       // integer type.
       // FIXME: Add support for range metadata here. Currently the utilities
       // for this don't propagate range metadata in trivial cases from one
if (NewBeginOffset == NewAllocaBeginOffset &&
                                      SI.isVolatile());
     }
     NewSI->copyMetadata(SI, {LLVMContext::MD_mem_parallel_loop_access,
                              LLVMContext::MD_access_group});
     if (AATags)
       NewSI->setAAMetadata(AATags);
     if (SI.isVolatile())
       NewSI->setAtomic(SI.getOrdering(), SI.getSyncScopeID());
+    if (NewSI->isAtomic())
+      NewSI->setAlignment(SI.getAlign());
     Pass.DeadInsts.insert(&SI);
     deleteIfTriviallyDead(OldOp);
 
     LLVM_DEBUG(dbgs() << "          to: " << *NewSI << "\n");
     return NewSI->getPointerOperand() == &NewAI && !SI.isVolatile();
   }
 
   /// Compute an integer value from splatting an i8 across the given

llvm/test/Transforms/SROA/alignment.ll

; CHECK: load i16, {{.*}}, align 2
 
   %ptr = alloca {i32, i8, i8, {i8, i16}}, align 2
   %ptr.8 = bitcast {i32, i8, i8, {i8, i16}}* %ptr to i8*
   call void @populate(i8* %ptr.8)
   %val = load {i32, i8, i8, {i8, i16}}, {i32, i8, i8, {i8, i16}}* %ptr, align 2
   ret void
 }
 
+%struct = type { i32, i32 }
+define dso_local i32 @pr45010(%struct* %A) {
+; CHECK-LABEL: @pr45010
+; CHECK: load atomic volatile i32, {{.*}}, align 4
+
+  %B = alloca %struct, align 4
+  %A.i = getelementptr inbounds %struct, %struct* %A, i32 0, i32 0
+  %B.i = getelementptr inbounds %struct, %struct* %B, i32 0, i32 0
+  %1 = load i32, i32* %A.i, align 4
+  store atomic volatile i32 %1, i32* %B.i release, align 4
+  %2 = bitcast %struct* %B to i32*
+  %x = load atomic volatile i32, i32* %2 acquire, align 4
+  ret i32 %x
+}
+
 declare void @populate(i8*)