Extend load/store type canonicalization to handle unordered operations

[reames]

Summary

The patch itself is pretty straight forward. I wanted to get a second set of eyes on this mostly to make sure there isn't some cornercase where the type of a unordered load or store effects the lowering in a semantic way. I'm not aware of any, but this is the change which would find them if they exist. :)

p.s. I believe we now support all power of two types for atomic loads and stores. If not, I'll need to add a restriction to the patch. Anyone know of a case?

[jfb]

I don't think it matters but would rather ask: there's no case where a less-than-naturally-aligned atomic would matter? I'm specifically thinking of an example such as:

%v = load atomic i32, i32* %p unordered, align 2
%c = bitcast i32 to float
%s = add float %c, 1.0

Could this new code be leading backends to generate bad code? I think not, LMK if you think otherwise.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

330

Add assert(!LI.isVolatile() && "volatile unhandled here") here and below, makes it less likely to break in the future.

test/Transforms/InstCombine/atomic.ll

207

; TODO: probably also legal in this case

[jfb]

Forgot to say: lgtm after these comments.

[reames]

This landed as 267210.

@JF - There shouldn't be any code generation differences for two types with the same alignment, even if that alignment isn't natural.

[jfb]

In D19381#409535, @reames wrote:

This landed as 267210.

@JF - There shouldn't be any code generation differences for two types with the same alignment, even if that alignment isn't natural.

Many ISAs have different load / store instructions for FP versus integer, and some behave differently. Going from FP to integer in this case (no intervening use) seems fine, but I do think it's likely to change code generation (unless the backend already performed the same transformation).

[chandlerc]

In D19381#409561, @jfb wrote:

In D19381#409535, @reames wrote:

This landed as 267210.

@JF - There shouldn't be any code generation differences for two types with the same alignment, even if that alignment isn't natural.

Many ISAs have different load / store instructions for FP versus integer, and some behave differently. Going from FP to integer in this case (no intervening use) seems fine, but I do think it's likely to change code generation (unless the backend already performed the same transformation).

The patch is good of course, but I think JF is onto something here. Specifically, it would be good to make sure that backends which implement reasonable lowering for under-aligned atomic load and store of an integer also reasonably lower under-aligned atomic load and store of a floating point value. And the same for pointers.

I'm worried there are targets that have never had anyone test their ability to lower a 1-byte aligned atomic float or atomic pointer, and will need to be updated in the face of this.

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static LoadInst *combineLoadToNewType(InstCombiner &IC, LoadInst &LI, Type *NewTy,
   Value *Ptr = LI.getPointerOperand();
   unsigned AS = LI.getPointerAddressSpace();
   SmallVector<std::pair<unsigned, MDNode *>, 8> MD;
   LI.getAllMetadata(MD);
 
   LoadInst *NewLoad = IC.Builder->CreateAlignedLoad(
       IC.Builder->CreateBitCast(Ptr, NewTy->getPointerTo(AS)),
       LI.getAlignment(), LI.getName() + Suffix);
+  NewLoad->setAtomic(LI.getOrdering(), LI.getSynchScope());
   MDBuilder MDB(NewLoad->getContext());
   for (const auto &MDPair : MD) {
     unsigned ID = MDPair.first;
     MDNode *N = MDPair.second;
     // Note, essentially every kind of metadata should be preserved here! This
     // routine is supposed to clone a load instruction changing *only its type*.
     // The only metadata it makes sense to drop is metadata which is invalidated
     // when the pointer type changes. This should essentially never be the case
static StoreInst *combineStoreToNewValue(InstCombiner &IC, StoreInst &SI, Value *V) {
   Value *Ptr = SI.getPointerOperand();
   unsigned AS = SI.getPointerAddressSpace();
   SmallVector<std::pair<unsigned, MDNode *>, 8> MD;
   SI.getAllMetadata(MD);
 
   StoreInst *NewStore = IC.Builder->CreateAlignedStore(
       V, IC.Builder->CreateBitCast(Ptr, V->getType()->getPointerTo(AS)),
       SI.getAlignment());
+  NewStore->setAtomic(SI.getOrdering(), SI.getSynchScope());
   for (const auto &MDPair : MD) {
     unsigned ID = MDPair.first;
     MDNode *N = MDPair.second;
     // Note, essentially every kind of metadata should be preserved here! This
     // routine is supposed to clone a store instruction changing *only its
     // type*. The only metadata it makes sense to drop is metadata which is
     // invalidated when the pointer type changes. This should essentially
     // never be the case in LLVM, but we explicitly switch over only known

 /// be a semantic no-op which just allows loads to more closely model the types
 /// of their consuming operations.
 ///
 /// Currently, we also refuse to change the precise type used for an atomic load
 /// or a volatile load. This is debatable, and might be reasonable to change
 /// later. However, it is risky in case some backend or other part of LLVM is
 /// relying on the exact type loaded to select appropriate atomic operations.
 static Instruction *combineLoadToOperationType(InstCombiner &IC, LoadInst &LI) {
-  // FIXME: We could probably with some care handle both volatile and atomic
-  // loads here but it isn't clear that this is important.
-  if (!LI.isSimple())
+  // FIXME: We could probably with some care handle both volatile and ordered
+  // atomic loads here but it isn't clear that this is important.
+  if (!LI.isUnordered())
     return nullptr;
 
   if (LI.use_empty())
     return nullptr;
 
   Type *Ty = LI.getType();
   const DataLayout &DL = IC.getDataLayout();
 

 /// However, it is risky in case some backend or other part of LLVM is relying
 /// on the exact type stored to select appropriate atomic operations.
 ///
 /// \returns true if the store was successfully combined away. This indicates
 /// the caller must erase the store instruction. We have to let the caller erase
 /// the store instruction as otherwise there is no way to signal whether it was
 /// combined or not: IC.EraseInstFromFunction returns a null pointer.
 static bool combineStoreToValueType(InstCombiner &IC, StoreInst &SI) {
-  // FIXME: We could probably with some care handle both volatile and atomic
-  // stores here but it isn't clear that this is important.
-  if (!SI.isSimple())
+  // FIXME: We could probably with some care handle both volatile and ordered
+  // atomic stores here but it isn't clear that this is important.
+  if (!SI.isUnordered())
     return false;
 
   Value *V = SI.getValueOperand();
 
   // Fold away bit casts of the stored value by storing the original type.
   if (auto *BC = dyn_cast<BitCastInst>(V)) {
     V = BC->getOperand(0);
     combineStoreToNewValue(IC, SI, V);

test/Transforms/InstCombine/atomic.ll

 ; FIXME: This would be legal to transform
 define i32 @test17(i1 %cnd) {
 ; CHECK-LABEL: define i32 @test17(
 ; CHECK: load atomic i32, i32* %addr seq_cst, align 4
   %addr = select i1 %cnd, i32* @a, i32* @b
   %x = load atomic i32, i32* %addr seq_cst, align 4
   ret i32 %x
 }
+
+declare void @clobber()
+
+define i32 @test18(float* %p) {
+; CHECK-LABEL: define i32 @test18(
+; CHECK: load atomic i32, i32* [[A:%.*]] unordered, align 4
+; CHECK: store atomic i32 [[B:%.*]], i32* [[C:%.*]] unordered, align 4
+  %x = load atomic float, float* %p unordered, align 4
+  call void @clobber() ;; keep the load around
+  store atomic float %x, float* %p unordered, align 4
+  ret i32 0
+}
+
+; TODO: probably also legal in this case
+define i32 @test19(float* %p) {
+; CHECK-LABEL: define i32 @test19(
+; CHECK: load atomic float, float* %p seq_cst, align 4
+; CHECK: store atomic float %x, float* %p seq_cst, align 4
+  %x = load atomic float, float* %p seq_cst, align 4
+  call void @clobber() ;; keep the load around
+  store atomic float %x, float* %p seq_cst, align 4
+  ret i32 0
+}
+
+define i32 @test20(i32** %p, i8* %v) {
+; CHECK-LABEL: define i32 @test20(
+; CHECK: store atomic i8* %v, i8** [[D:%.*]] unordered, align 4
+  %cast = bitcast i8* %v to i32*
+  store atomic i32* %cast, i32** %p unordered, align 4
+  ret i32 0
+}
+
+define i32 @test21(i32** %p, i8* %v) {
+; CHECK-LABEL: define i32 @test21(
+; CHECK: store atomic i32* %cast, i32** %p monotonic, align 4
+  %cast = bitcast i8* %v to i32*
+  store atomic i32* %cast, i32** %p monotonic, align 4
+  ret i32 0
+}