[AtomicExpand] Make floating point conversion happens before fence insertion

[lkail]

Summary

IIUC, the conversion part is not part of atomic operations and fences should be put around converted atomic operations.
This also fixes atomic load of floating point values which requires fence on PowerPC.

[efriedma]

llvm/lib/CodeGen/AtomicExpandPass.cpp

226

I think you can drop the TODOs here, at least for load/store/atomicrmw.

llvm/test/CodeGen/PowerPC/cfence-double.ll

16

Please avoid using undef in testcases like this.

[arsenm]

llvm/lib/CodeGen/AtomicExpandPass.cpp

229

Don't see much point in this assert (plus the others)

253–254

There's no reason to assert on things that fail the verifier

llvm/test/CodeGen/PowerPC/cfence-double.ll

8

Should have an IR test in test/Transforms/AtomicExpand too

[lkail]

Address comments.

[lkail]

Ping.

[efriedma]

LGTM

[lkail]

Rebased tests.

llvm/lib/CodeGen/AtomicExpandPass.cpp

for (auto I : AtomicInsts) {
     } else if (CASI) {
       if (!atomicSizeSupported(TLI, CASI)) {
         expandAtomicCASToLibcall(CASI);
         MadeChange = true;
         continue;
       }
     }
 
+    if (LI && TLI->shouldCastAtomicLoadInIR(LI) ==
+                  TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
+      // TODO: add a TLI hook to control this so that each target can
+      // convert to lowering the original type one at a time.
+      I = LI = convertAtomicLoadToIntegerType(LI);
+      assert(LI->getType()->isIntegerTy() && "invariant broken");
+      MadeChange = true;
+    } else if (SI &&
+               TLI->shouldCastAtomicStoreInIR(SI) ==
+                   TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
+      // TODO: add a TLI hook to control this so that each target can
+      // convert to lowering the original type one at a time.
+      I = SI = convertAtomicStoreToIntegerType(SI);
+      assert(SI->getValueOperand()->getType()->isIntegerTy() &&
+             "invariant broken");
+      MadeChange = true;
+
+    } else if (RMWI &&
+               TLI->shouldCastAtomicRMWIInIR(RMWI) ==
+                   TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
+      // TODO: add a TLI hook to control this so that each target can
+      // convert to lowering the original type one at a time.
+      I = RMWI = convertAtomicXchgToIntegerType(RMWI);
+      assert(RMWI->getValOperand()->getType()->isIntegerTy() &&
+             "invariant broken");
+      MadeChange = true;
+    } else if (CASI) {
+      // TODO: when we're ready to make the change at the IR level, we can
+      // extend convertCmpXchgToInteger for floating point too.
+      assert(!CASI->getCompareOperand()->getType()->isFloatingPointTy() &&
+             "unimplemented - floating point not legal at IR level");
+      if (CASI->getCompareOperand()->getType()->isPointerTy()) {
+        // TODO: add a TLI hook to control this so that each target can
+        // convert to lowering the original type one at a time.
+        I = CASI = convertCmpXchgToIntegerType(CASI);
+        assert(CASI->getCompareOperand()->getType()->isIntegerTy() &&
+               "invariant broken");
+        MadeChange = true;
+      }
+    }
+
     if (TLI->shouldInsertFencesForAtomic(I)) {
       auto FenceOrdering = AtomicOrdering::Monotonic;
       if (LI && isAcquireOrStronger(LI->getOrdering())) {
         FenceOrdering = LI->getOrdering();
         LI->setOrdering(AtomicOrdering::Monotonic);
       } else if (SI && isReleaseOrStronger(SI->getOrdering())) {
         FenceOrdering = SI->getOrdering();
         SI->setOrdering(AtomicOrdering::Monotonic);
if (TLI->shouldInsertFencesForAtomic(I)) {
         CASI->setFailureOrdering(AtomicOrdering::Monotonic);
       }
 
       if (FenceOrdering != AtomicOrdering::Monotonic) {
         MadeChange |= bracketInstWithFences(I, FenceOrdering);
       }
     }
 
-    if (LI) {
-      if (TLI->shouldCastAtomicLoadInIR(LI) ==
-          TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
-        // TODO: add a TLI hook to control this so that each target can
-        // convert to lowering the original type one at a time.
-        LI = convertAtomicLoadToIntegerType(LI);
-        assert(LI->getType()->isIntegerTy() && "invariant broken");
-        MadeChange = true;
-      }
-
+    if (LI)
       MadeChange |= tryExpandAtomicLoad(LI);
-    } else if (SI) {
-      if (TLI->shouldCastAtomicStoreInIR(SI) ==
-          TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
-        // TODO: add a TLI hook to control this so that each target can
-        // convert to lowering the original type one at a time.
-        SI = convertAtomicStoreToIntegerType(SI);
-        assert(SI->getValueOperand()->getType()->isIntegerTy() &&
-               "invariant broken");
-        MadeChange = true;
-      }
-
-      if (tryExpandAtomicStore(SI))
-        MadeChange = true;
-    } else if (RMWI) {
+    else if (SI)
+      MadeChange |= tryExpandAtomicStore(SI);
+    else if (RMWI) {
       // There are two different ways of expanding RMW instructions:
       // - into a load if it is idempotent
       // - into a Cmpxchg/LL-SC loop otherwise
       // we try them in that order.
 
       if (isIdempotentRMW(RMWI) && simplifyIdempotentRMW(RMWI)) {
         MadeChange = true;
       } else {
         AtomicRMWInst::BinOp Op = RMWI->getOperation();
-        if (TLI->shouldCastAtomicRMWIInIR(RMWI) ==
-            TargetLoweringBase::AtomicExpansionKind::CastToInteger) {
-          // TODO: add a TLI hook to control this so that each target can
-          // convert to lowering the original type one at a time.
-          RMWI = convertAtomicXchgToIntegerType(RMWI);
-          assert(RMWI->getValOperand()->getType()->isIntegerTy() &&
-                 "invariant broken");
-          MadeChange = true;
-        }
         unsigned MinCASSize = TLI->getMinCmpXchgSizeInBits() / 8;
         unsigned ValueSize = getAtomicOpSize(RMWI);
         if (ValueSize < MinCASSize &&
             (Op == AtomicRMWInst::Or || Op == AtomicRMWInst::Xor ||
              Op == AtomicRMWInst::And)) {
           RMWI = widenPartwordAtomicRMW(RMWI);
           MadeChange = true;
         }
 
         MadeChange |= tryExpandAtomicRMW(RMWI);
       }
-    } else if (CASI) {
-      // TODO: when we're ready to make the change at the IR level, we can
-      // extend convertCmpXchgToInteger for floating point too.
-      assert(!CASI->getCompareOperand()->getType()->isFloatingPointTy() &&
-             "unimplemented - floating point not legal at IR level");
-      if (CASI->getCompareOperand()->getType()->isPointerTy()) {
-        // TODO: add a TLI hook to control this so that each target can
-        // convert to lowering the original type one at a time.
-        CASI = convertCmpXchgToIntegerType(CASI);
-        assert(CASI->getCompareOperand()->getType()->isIntegerTy() &&
-               "invariant broken");
-        MadeChange = true;
-      }
-
+    } else if (CASI)
       MadeChange |= tryExpandAtomicCmpXchg(CASI);
-    }
-  }
   return MadeChange;
 }
 
 bool AtomicExpand::bracketInstWithFences(Instruction *I, AtomicOrdering Order) {
   IRBuilder<> Builder(I);
 
   auto LeadingFence = TLI->emitLeadingFence(Builder, I, Order);
 

llvm/test/CodeGen/PowerPC/cfence-double.ll

-; RUN: not --crash llc -opaque-pointers -mtriple=powerpc64le-unknown-unknown \
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -opaque-pointers -mtriple=powerpc64le-unknown-unknown \
 ; RUN:   < %s 2>&1 | FileCheck %s
-; RUN: not --crash llc -opaque-pointers -mtriple=powerpc64-unknown-unknown \
+; RUN: llc -opaque-pointers -mtriple=powerpc64-unknown-unknown \
 ; RUN:   < %s 2>&1 | FileCheck %s
 
-; CHECK: Intrinsic has incorrect argument type!
-; CHECK: ptr @llvm.ppc.cfence.f64
 define void @foo() {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    ld 3, 0(3)
+; CHECK-NEXT:    cmpd 7, 3, 3
+; CHECK-NEXT:    bne- 7, .+4
+; CHECK-NEXT:    isync
+; CHECK-NEXT:    blr
 entry:
   %0 = load atomic double, double* undef acquire, align 8
   ret void
 }

llvm/test/CodeGen/PowerPC/cfence-float.ll

-; RUN: not --crash llc -opaque-pointers -mtriple=powerpc64le-unknown-unknown \
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -opaque-pointers -mtriple=powerpc64le-unknown-unknown \
 ; RUN:   < %s 2>&1 | FileCheck %s
-; RUN: not --crash llc -opaque-pointers -mtriple=powerpc64-unknown-unknown \
+; RUN: llc -opaque-pointers -mtriple=powerpc64-unknown-unknown \
 ; RUN:   < %s 2>&1 | FileCheck %s
 
-; CHECK: Intrinsic has incorrect argument type!
-; CHECK: ptr @llvm.ppc.cfence.f32
 define void @bar() {
+; CHECK-LABEL: bar:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    lwz 3, 0(3)
+; CHECK-NEXT:    cmpd 7, 3, 3
+; CHECK-NEXT:    bne- 7, .+4
+; CHECK-NEXT:    isync
+; CHECK-NEXT:    blr
 entry:
   %0 = load atomic float, float* undef acquire, align 8
   ret void
 }