Mutate TargetLowering::shouldExpandAtomicRMWInIR to specifically dictate how AtomicRMWInsts are expanded.

[DiamondLovesYou]

Summary

In PNaCl, most atomic instructions have their own @llvm.nacl.atomic.* function, each one, with a few exceptions, represents a consistent behaviour across all NaCl-supported targets. Unfortunately, the atomic RMW operations nand, [u]min, and [u]max aren't directly represented by any such @llvm.nacl.atomic.* function. This patch refines shouldExpandAtomicRMWInIR in TargetLowering so that a future Le32TargetLowering class can selectively inform the caller how the target desires the atomic RMW instruction to be expanded (ie via load-linked/store-conditional for ARM/AArch64, via cmpxchg for X86/others?, or not at all for Mips) if at all.

This does not represent a behavioural change and as such no tests were added.

[t.p.northover]

No tests? No explanation?

include/llvm/Target/TargetLowering.h

1088

Does any compiler really whinge about this?

[DiamondLovesYou]

In D7713#125228, @t.p.northover wrote:

No tests?

It seems to me that the test would be passing existing tests, considering this patch happens only at an API boundary and doesn't change the behaviour of existing code.

No explanation?

Apologies; I've added a summary.

[jfb]

In D7713#125473, @DiamondLovesYou wrote:

In D7713#125228, @t.p.northover wrote:

No tests?

It seems to me that the test would be passing existing tests, considering this patch happens only at an API boundary and doesn't change the behaviour of existing code.

You change description should explain that this change doesn't lead to a functional change, so the existing tests are all still valid.

include/llvm/Target/TargetLowering.h

128

I would just call this None instead of Native.

lib/CodeGen/AtomicExpandPass.cpp

143

I find the logic with continue confusing, could you instead do:

if (...) {
  MadeChange = true;
} else {
  MadeChange |= ...;
}

226–227

I'd rename to tryExpandAtomicRMW since it doesn't expand them unconditionally anymore.

227

clang-format.

I'd also just do:

switch (TLI->shouldExpandAtomicRMWInIR(AI)) {

233

Use string concatenation instead of backslashes:

"foo "
"bar"

Is equivalent to:

"foo bar"

239

Could you explain the rationale for splitting LLSC from CmpXhg instead of using the same logic as before? It does provide more flexibility, but the enum doesn't really explain why that flexibility is desirable, and doesn't point out that LLSC implies hasLoadLinkedStoreConditional: even if your assert checks this, it's a tricky gotcha for a Target implementer.

lib/Target/AArch64/AArch64ISelLowering.cpp

8781

I think it's generally more idiomatic to do:

return Size <= 128 ? LLSC : Native;

lib/Target/ARM/ARMISelLowering.cpp

11109

Same on ternary operator.

lib/Target/X86/X86ISelLowering.cpp

19622

Same.

19638

Same.

[DiamondLovesYou]

Addressed JF's comments.

[jfb]

Looks good overall, but I'll let @t.p.northover comment more.

lib/CodeGen/AtomicExpandPass.cpp

218

Why remove this?

[DiamondLovesYou]

Fix a comment from the last patchset.

[jfb]

This lgtm, I'll leave open for a short while if anyone else wants to comment.

[jfb]

@DiamondLovesYou: could you re-upload this patch? It looks like the latest one is an incremental patch comparing your previous one, not a patch from tip-of-tree. Once that's done I'll be able to commit.

[DiamondLovesYou]

Fix use of an incremental diff.

[jfb]

I also re-ran clang-format, some of the code wasn't formatted with it.

lib/CodeGen/AtomicExpandPass.cpp

228

I removed the semicolon here, and had to add llvm_unreachable because not all compilers realize that all cases are handled.

include/llvm/Target/TargetLowering.h

public:
   enum SelectSupportKind {
     ScalarValSelect,      // The target supports scalar selects (ex: cmov).
     ScalarCondVectorVal,  // The target supports selects with a scalar condition
                           // and vector values (ex: cmov).
     VectorMaskSelect      // The target supports vector selects with a vector
                           // mask (ex: x86 blends).
   };
 
-  /// Enum that specifies what a AtomicRMWInst is expanded to, if at all.
+  /// Enum that specifies what a AtomicRMWInst is expanded to, if at all. Exists
+  /// because different targets have different levels of support for these
+  /// atomic RMW instructions, and also have different options w.r.t. what they should
+  /// expand to.
   enum class AtomicRMWExpansionKind {
-    Native,    // Don't expand the instruction.
-    LLSC,      // Expand the instruction into loadlinked/storeconditional.
-    CmpXChg,   // Expand the instruction into cmpxchg.
+    None,      // Don't expand the instruction.
+    LLSC,      // Expand the instruction into loadlinked/storeconditional; used
+               // by ARM/AArch64. Implies `hasLoadLinkedStoreConditional`
+               // returns true.
+    CmpXChg,   // Expand the instruction into cmpxchg; used by at least X86.
   };
 
   static ISD::NodeType getExtendForContent(BooleanContent Content) {
     switch (Content) {
     case UndefinedBooleanContent:
       // Extend by adding rubbish bits.
       return ISD::ANY_EXTEND;
     case ZeroOrOneBooleanContent:
public:
   /// Returns true if the given (atomic) load should be expanded by the
   /// IR-level AtomicExpand pass into a load-linked instruction
   /// (through emitLoadLinked()).
   virtual bool shouldExpandAtomicLoadInIR(LoadInst *LI) const { return false; }
 
   /// Returns how the IR-level AtomicExpand pass should expand the given
   /// AtomicRMW, if at all. Default is to never expand.
   virtual AtomicRMWExpansionKind shouldExpandAtomicRMWInIR(AtomicRMWInst *RMWI) const {
     (void)RMWI;
-    return AtomicRMWExpansionKind::Native;
+    return AtomicRMWExpansionKind::None;
   }
 
   /// On some platforms, an AtomicRMW that never actually modifies the value
   /// (such as fetch_add of 0) can be turned into a fence followed by an
   /// atomic load. This may sound useless, but it makes it possible for the
   /// processor to keep the cacheline shared, dramatically improving
   /// performance. And such idempotent RMWs are useful for implementing some
   /// kinds of locks, see for example (justification + benchmarks):

lib/CodeGen/AtomicExpandPass.cpp

namespace {
 
   private:
     bool bracketInstWithFences(Instruction *I, AtomicOrdering Order,
                                bool IsStore, bool IsLoad);
     bool expandAtomicLoad(LoadInst *LI);
     bool expandAtomicLoadToLL(LoadInst *LI);
     bool expandAtomicLoadToCmpXchg(LoadInst *LI);
     bool expandAtomicStore(StoreInst *SI);
-    bool expandAtomicRMW(AtomicRMWInst *AI);
+    bool tryExpandAtomicRMW(AtomicRMWInst *AI);
     bool expandAtomicRMWToLLSC(AtomicRMWInst *AI);
     bool expandAtomicRMWToCmpXchg(AtomicRMWInst *AI);
     bool expandAtomicCmpXchg(AtomicCmpXchgInst *CI);
     bool isIdempotentRMW(AtomicRMWInst *AI);
     bool simplifyIdempotentRMW(AtomicRMWInst *AI);
   };
 }
 
for (auto I : AtomicInsts) {
     } 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;
-        continue;
+      } else {
+        MadeChange |= tryExpandAtomicRMW(RMWI);
       }
-      MadeChange |= expandAtomicRMW(RMWI);
     } else if (CASI && TLI->hasLoadLinkedStoreConditional()) {
       MadeChange |= expandAtomicCmpXchg(CASI);
     }
   }
   return MadeChange;
 }
 
 bool AtomicExpand::bracketInstWithFences(Instruction *I, AtomicOrdering Order,
bool AtomicExpand::expandAtomicLoadToCmpXchg(LoadInst *LI) {
   return true;
 }
 
 bool AtomicExpand::expandAtomicStore(StoreInst *SI) {
   // This function is only called on atomic stores that are too large to be
   // atomic if implemented as a native store. So we replace them by an
   // atomic swap, that can be implemented for example as a ldrex/strex on ARM
   // or lock cmpxchg8/16b on X86, as these are atomic for larger sizes.
-  // It is the responsibility of the target to only return true in
-  // shouldExpandAtomicRMW in cases where this is required and possible.
   IRBuilder<> Builder(SI);
   AtomicRMWInst *AI =
       Builder.CreateAtomicRMW(AtomicRMWInst::Xchg, SI->getPointerOperand(),
                               SI->getValueOperand(), SI->getOrdering());
   SI->eraseFromParent();
 
   // Now we have an appropriate swap instruction, lower it as usual.
-  return expandAtomicRMW(AI);
+  return tryExpandAtomicRMW(AI);
 }
 
-bool AtomicExpand::expandAtomicRMW(AtomicRMWInst *AI) {
-  const auto How = TLI->shouldExpandAtomicRMWInIR(AI);
-  switch(How) {
-    case TargetLoweringBase::AtomicRMWExpansionKind::Native: return false;
-    case TargetLoweringBase::AtomicRMWExpansionKind::LLSC: {
-      assert(TLI->hasLoadLinkedStoreConditional() &&
-             "TargetLowering requested we expand AtomicRMW instruction into \
-              load-linked/store-conditional combos, but such instruction aren't \
-              supported");
+bool AtomicExpand::tryExpandAtomicRMW(AtomicRMWInst *AI) {
+  switch (TLI->shouldExpandAtomicRMWInIR(AI)) {
+  case TargetLoweringBase::AtomicRMWExpansionKind::None:
+    return false;
+           "TargetLowering requested we expand AtomicRMW instruction into "
+           "load-linked/store-conditional combos, but such instructions aren't "
+           "supported");
 
-      return expandAtomicRMWToLLSC(AI);
-    }
-    case TargetLoweringBase::AtomicRMWExpansionKind::CmpXChg: {
-      return expandAtomicRMWToCmpXchg(AI);
-    }
   };
 }
 
 /// Emit IR to implement the given atomicrmw operation on values in registers,
 /// returning the new value.
 static Value *performAtomicOp(AtomicRMWInst::BinOp Op, IRBuilder<> &Builder,
                               Value *Loaded, Value *Inc) {
   Value *NewVal;

lib/Target/AArch64/AArch64ISelLowering.cpp

bool AArch64TargetLowering::shouldExpandAtomicLoadInIR(LoadInst *LI) const {
   unsigned Size = LI->getType()->getPrimitiveSizeInBits();
   return Size == 128;
 }
 
 // For the real atomic operations, we have ldxr/stxr up to 128 bits,
 TargetLoweringBase::AtomicRMWExpansionKind
 AArch64TargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
   unsigned Size = AI->getType()->getPrimitiveSizeInBits();
-  if (Size <= 128) {
-    return TargetLoweringBase::AtomicRMWExpansionKind::LLSC;
-  } else {
-    return TargetLoweringBase::AtomicRMWExpansionKind::Native;
-  }
+  return Size <= 128 ?
+    AtomicRMWExpansionKind::LLSC : AtomicRMWExpansionKind::None;
 }
 
 bool AArch64TargetLowering::hasLoadLinkedStoreConditional() const {
   return true;
 }
 
 Value *AArch64TargetLowering::emitLoadLinked(IRBuilder<> &Builder, Value *Addr,
                                              AtomicOrdering Ord) const {
   Module *M = Builder.GetInsertBlock()->getParent()->getParent();
   Type *ValTy = cast<PointerType>(Addr->getType())->getElementType();

lib/Target/ARM/ARMISelLowering.cpp

bool ARMTargetLowering::shouldExpandAtomicLoadInIR(LoadInst *LI) const {
   return (Size == 64) && !Subtarget->isMClass();
 }
 
 // For the real atomic operations, we have ldrex/strex up to 32 bits,
 // and up to 64 bits on the non-M profiles
 TargetLoweringBase::AtomicRMWExpansionKind
 ARMTargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
   unsigned Size = AI->getType()->getPrimitiveSizeInBits();
-  if (Size <= (Subtarget->isMClass() ? 32U : 64U)) {
-    return TargetLoweringBase::AtomicRMWExpansionKind::LLSC;
-  } else {
-    return TargetLoweringBase::AtomicRMWExpansionKind::Native;
-  }
+  return (Size <= (Subtarget->isMClass() ? 32U : 64U)) ?
+    AtomicRMWExpansionKind::LLSC : AtomicRMWExpansionKind::None;
 }
 
 // This has so far only been implemented for MachO.
 bool ARMTargetLowering::useLoadStackGuardNode() const {
   return Subtarget->isTargetMachO();
 }
 
 bool ARMTargetLowering::canCombineStoreAndExtract(Type *VectorTy, Value *Idx,
                                                   unsigned &Cost) const {
   // If we do not have NEON, vector types are not natively supported.

lib/Target/X86/X86ISelLowering.cpp

 TargetLoweringBase::AtomicRMWExpansionKind
 X86TargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
   unsigned NativeWidth = Subtarget->is64Bit() ? 64 : 32;
   const Type *MemType = AI->getType();
 
   // If the operand is too big, we must see if cmpxchg8/16b is available
   // and default to library calls otherwise.
   if (MemType->getPrimitiveSizeInBits() > NativeWidth) {
-    if (needsCmpXchgNb(MemType))
-      return TargetLoweringBase::AtomicRMWExpansionKind::CmpXChg;
-    else
-      return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+    return needsCmpXchgNb(MemType) ?
+      AtomicRMWExpansionKind::CmpXChg : AtomicRMWExpansionKind::None;
   }
 
   AtomicRMWInst::BinOp Op = AI->getOperation();
   switch (Op) {
   default:
     llvm_unreachable("Unknown atomic operation");
   case AtomicRMWInst::Xchg:
   case AtomicRMWInst::Add:
   case AtomicRMWInst::Sub:
     // It's better to use xadd, xsub or xchg for these in all cases.
-    return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+    return AtomicRMWExpansionKind::None;
   case AtomicRMWInst::Or:
   case AtomicRMWInst::And:
   case AtomicRMWInst::Xor:
     // If the atomicrmw's result isn't actually used, we can just add a "lock"
     // prefix to a normal instruction for these operations.
-    if (!AI->use_empty())
-      return TargetLoweringBase::AtomicRMWExpansionKind::CmpXChg;
-    else
-      return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+    return !AI->use_empty() ?
+      AtomicRMWExpansionKind::CmpXChg : AtomicRMWExpansionKind::None;
   case AtomicRMWInst::Nand:
   case AtomicRMWInst::Max:
   case AtomicRMWInst::Min:
   case AtomicRMWInst::UMax:
   case AtomicRMWInst::UMin:
     // These always require a non-trivial set of data operations on x86. We must
     // use a cmpxchg loop.
-    return TargetLoweringBase::AtomicRMWExpansionKind::CmpXChg;
+    return AtomicRMWExpansionKind::CmpXChg;
   }
 }
 
 static bool hasMFENCE(const X86Subtarget& Subtarget) {
   // Use mfence if we have SSE2 or we're on x86-64 (even if we asked for
   // no-sse2). There isn't any reason to disable it if the target processor
   // supports it.
   return Subtarget.hasSSE2() || Subtarget.is64Bit();