Transforms: Fix code duplication between LowerAtomic and AtomicExpand

[reames]

After you've added the NotAtomic option to AtomicExpand, does LowerAtomic serve any purpose at all? The one in tree use I can find (in ARM) could be handled via the shouldExpandX callbacks without issue (I think). If possible, I'd rather switch that one backend over and then just delete the code.

Unless we think the ability to strip out atomics is useful for debugging or something?

[arsenm]

In D123172#3434681, @reames wrote:

After you've added the NotAtomic option to AtomicExpand, does LowerAtomic serve any purpose at all? The one in tree use I can find (in ARM) could be handled via the shouldExpandX callbacks without issue (I think). If possible, I'd rather switch that one backend over and then just delete the code.

NVPTX has another conditionalized version of the pass which could be moved to AtomicExpand. WebAssembly also has a use where they use the pass itself inside another pass. The main annoyance is the clumsy API where each instruction gets its own callback

Unless we think the ability to strip out atomics is useful for debugging or something?

I was thinking it's not useful unless used as a utility, but not sure if there's a real point to that.

[reames]

Good point on your response.

LGTM to this patch as a stepping stone.

I really do think it makes sense to cleanup the aarch64 usage given the new capability. Would you mind doing that? Not urgent, but it'd be good not to forget.

The others really seem like we need a function which lowers an inst to it's non-atomic form, and call it rather than invoking the pass as a helper. Feel free to pursue that or not, we're at the point of diminishing returns here.

[arsenm]

Pushed a long time ago as 9fdd25848a79892eaacc5414d5aef18555b79919 and I never got to the further cleanups

llvm/include/llvm/Transforms/Utils/LowerAtomic.h

 //===- LowerAtomic.h - Lower atomic intrinsics ------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
 // This pass lowers atomic intrinsics to non-atomic form for use in a known
 // non-preemptible environment.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_SCALAR_LOWERATOMIC_H
 #define LLVM_TRANSFORMS_SCALAR_LOWERATOMIC_H
 
+#include "llvm/IR/Instructions.h"
+
 namespace llvm {
-class AtomicCmpXchgInst;
-class AtomicRMWInst;
+
+class IRBuilderBase;
 
 /// Convert the given Cmpxchg into primitive load and compare.
 bool lowerAtomicCmpXchgInst(AtomicCmpXchgInst *CXI);
 
 /// Convert the given RMWI into primitive load and stores,
 /// assuming that doing so is legal. Return true if the lowering
 /// succeeds.
 bool lowerAtomicRMWInst(AtomicRMWInst *RMWI);
+
+/// Emit IR to implement the given atomicrmw operation on values in registers,
+/// returning the new value.
+Value *buildAtomicRMWValue(AtomicRMWInst::BinOp Op, IRBuilderBase &Builder,
+                           Value *Loaded, Value *Inc);
 }
 
 #endif // LLVM_TRANSFORMS_SCALAR_LOWERATOMIC_H

llvm/lib/CodeGen/AtomicExpandPass.cpp

Value *Pair = Builder.CreateAtomicCmpXchg(
       AtomicCmpXchgInst::getStrongestFailureOrdering(MemOpOrder), SSID);
   Success = Builder.CreateExtractValue(Pair, 1, "success");
   NewLoaded = Builder.CreateExtractValue(Pair, 0, "newloaded");
 
   if (NeedBitcast)
     NewLoaded = Builder.CreateBitCast(NewLoaded, OrigTy);
 }
 
-/// 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;
-  switch (Op) {
-  case AtomicRMWInst::Xchg:
-    return Inc;
-  case AtomicRMWInst::Add:
-    return Builder.CreateAdd(Loaded, Inc, "new");
-  case AtomicRMWInst::Sub:
-    return Builder.CreateSub(Loaded, Inc, "new");
-  case AtomicRMWInst::And:
-    return Builder.CreateAnd(Loaded, Inc, "new");
-  case AtomicRMWInst::Nand:
-    return Builder.CreateNot(Builder.CreateAnd(Loaded, Inc), "new");
-  case AtomicRMWInst::Or:
-    return Builder.CreateOr(Loaded, Inc, "new");
-  case AtomicRMWInst::Xor:
-    return Builder.CreateXor(Loaded, Inc, "new");
-  case AtomicRMWInst::Max:
-    NewVal = Builder.CreateICmpSGT(Loaded, Inc);
-    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
-  case AtomicRMWInst::Min:
-    NewVal = Builder.CreateICmpSLE(Loaded, Inc);
-    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
-  case AtomicRMWInst::UMax:
-    NewVal = Builder.CreateICmpUGT(Loaded, Inc);
-    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
-  case AtomicRMWInst::UMin:
-    NewVal = Builder.CreateICmpULE(Loaded, Inc);
-    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
-  case AtomicRMWInst::FAdd:
-    return Builder.CreateFAdd(Loaded, Inc, "new");
-  case AtomicRMWInst::FSub:
-    return Builder.CreateFSub(Loaded, Inc, "new");
-  default:
-    llvm_unreachable("Unknown atomic op");
-  }
-}
-
 bool AtomicExpand::tryExpandAtomicRMW(AtomicRMWInst *AI) {
   LLVMContext &Ctx = AI->getModule()->getContext();
   TargetLowering::AtomicExpansionKind Kind = TLI->shouldExpandAtomicRMWInIR(AI);
   switch (Kind) {
   case TargetLoweringBase::AtomicExpansionKind::None:
     return false;
   case TargetLoweringBase::AtomicExpansionKind::LLSC: {
     unsigned MinCASSize = TLI->getMinCmpXchgSizeInBits() / 8;
     unsigned ValueSize = getAtomicOpSize(AI);
     if (ValueSize < MinCASSize) {
       expandPartwordAtomicRMW(AI,
                               TargetLoweringBase::AtomicExpansionKind::LLSC);
     } else {
       auto PerformOp = [&](IRBuilder<> &Builder, Value *Loaded) {
-        return performAtomicOp(AI->getOperation(), Builder, Loaded,
+        return buildAtomicRMWValue(AI->getOperation(), Builder, Loaded,
                                    AI->getValOperand());
       };
       expandAtomicOpToLLSC(AI, AI->getType(), AI->getPointerOperand(),
                            AI->getAlign(), AI->getOrdering(), PerformOp);
     }
     return true;
   }
   case TargetLoweringBase::AtomicExpansionKind::CmpXChg: {
     unsigned MinCASSize = TLI->getMinCmpXchgSizeInBits() / 8;
static Value *performMaskedAtomicOp(AtomicRMWInst::BinOp Op,
   case AtomicRMWInst::Or:
   case AtomicRMWInst::Xor:
   case AtomicRMWInst::And:
     llvm_unreachable("Or/Xor/And handled by widenPartwordAtomicRMW");
   case AtomicRMWInst::Add:
   case AtomicRMWInst::Sub:
   case AtomicRMWInst::Nand: {
     // The other arithmetic ops need to be masked into place.
-    Value *NewVal = performAtomicOp(Op, Builder, Loaded, Shifted_Inc);
+    Value *NewVal = buildAtomicRMWValue(Op, Builder, Loaded, Shifted_Inc);
     Value *NewVal_Masked = Builder.CreateAnd(NewVal, PMV.Mask);
     Value *Loaded_MaskOut = Builder.CreateAnd(Loaded, PMV.Inv_Mask);
     Value *FinalVal = Builder.CreateOr(Loaded_MaskOut, NewVal_Masked);
     return FinalVal;
   }
   case AtomicRMWInst::Max:
   case AtomicRMWInst::Min:
   case AtomicRMWInst::UMax:
   case AtomicRMWInst::UMin: {
     // Finally, comparison ops will operate on the full value, so
     // truncate down to the original size, and expand out again after
     // doing the operation.
     Value *Loaded_Extract = extractMaskedValue(Builder, Loaded, PMV);
-    Value *NewVal = performAtomicOp(Op, Builder, Loaded_Extract, Inc);
+    Value *NewVal = buildAtomicRMWValue(Op, Builder, Loaded_Extract, Inc);
     Value *FinalVal = insertMaskedValue(Builder, Loaded, NewVal, PMV);
     return FinalVal;
   }
   default:
     llvm_unreachable("Unknown atomic op");
   }
 }
 

 // Note: This function is exposed externally by AtomicExpandUtils.h
 bool llvm::expandAtomicRMWToCmpXchg(AtomicRMWInst *AI,
                                     CreateCmpXchgInstFun CreateCmpXchg) {
   IRBuilder<> Builder(AI);
   Value *Loaded = AtomicExpand::insertRMWCmpXchgLoop(
       Builder, AI->getType(), AI->getPointerOperand(), AI->getAlign(),
       AI->getOrdering(), AI->getSyncScopeID(),
       [&](IRBuilder<> &Builder, Value *Loaded) {
-        return performAtomicOp(AI->getOperation(), Builder, Loaded,
+        return buildAtomicRMWValue(AI->getOperation(), Builder, Loaded,
                                    AI->getValOperand());
       },
       CreateCmpXchg);
 
   AI->replaceAllUsesWith(Loaded);
   AI->eraseFromParent();
   return true;
 }
 

llvm/lib/Transforms/Utils/LowerAtomic.cpp

bool llvm::lowerAtomicCmpXchgInst(AtomicCmpXchgInst *CXI) {
   Res = Builder.CreateInsertValue(UndefValue::get(CXI->getType()), Orig, 0);
   Res = Builder.CreateInsertValue(Res, Equal, 1);
 
   CXI->replaceAllUsesWith(Res);
   CXI->eraseFromParent();
   return true;
 }
 
-bool llvm::lowerAtomicRMWInst(AtomicRMWInst *RMWI) {
-  IRBuilder<> Builder(RMWI);
-  Value *Ptr = RMWI->getPointerOperand();
-  Value *Val = RMWI->getValOperand();
-
-  LoadInst *Orig = Builder.CreateLoad(Val->getType(), Ptr);
-  Value *Res = nullptr;
-
-  switch (RMWI->getOperation()) {
-  default: llvm_unreachable("Unexpected RMW operation");
+Value *llvm::buildAtomicRMWValue(AtomicRMWInst::BinOp Op,
+                                 IRBuilderBase &Builder, Value *Loaded,
+                                 Value *Inc) {
+  Value *NewVal;
+  switch (Op) {
   case AtomicRMWInst::Xchg:
-    Res = Val;
-    break;
+    return Inc;
   case AtomicRMWInst::Add:
-    Res = Builder.CreateAdd(Orig, Val);
-    break;
+    return Builder.CreateAdd(Loaded, Inc, "new");
   case AtomicRMWInst::Sub:
-    Res = Builder.CreateSub(Orig, Val);
-    break;
+    return Builder.CreateSub(Loaded, Inc, "new");
   case AtomicRMWInst::And:
-    Res = Builder.CreateAnd(Orig, Val);
-    break;
+    return Builder.CreateAnd(Loaded, Inc, "new");
   case AtomicRMWInst::Nand:
-    Res = Builder.CreateNot(Builder.CreateAnd(Orig, Val));
-    break;
+    return Builder.CreateNot(Builder.CreateAnd(Loaded, Inc), "new");
   case AtomicRMWInst::Or:
-    Res = Builder.CreateOr(Orig, Val);
-    break;
+    return Builder.CreateOr(Loaded, Inc, "new");
   case AtomicRMWInst::Xor:
-    Res = Builder.CreateXor(Orig, Val);
-    break;
+    return Builder.CreateXor(Loaded, Inc, "new");
   case AtomicRMWInst::Max:
-    Res = Builder.CreateSelect(Builder.CreateICmpSLT(Orig, Val),
-                               Val, Orig);
-    break;
+    NewVal = Builder.CreateICmpSGT(Loaded, Inc);
+    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
   case AtomicRMWInst::Min:
-    Res = Builder.CreateSelect(Builder.CreateICmpSLT(Orig, Val),
-                               Orig, Val);
-    break;
+    NewVal = Builder.CreateICmpSLE(Loaded, Inc);
+    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
   case AtomicRMWInst::UMax:
-    Res = Builder.CreateSelect(Builder.CreateICmpULT(Orig, Val),
-                               Val, Orig);
-    break;
+    NewVal = Builder.CreateICmpUGT(Loaded, Inc);
+    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
   case AtomicRMWInst::UMin:
-    Res = Builder.CreateSelect(Builder.CreateICmpULT(Orig, Val),
-                               Orig, Val);
-    break;
+    NewVal = Builder.CreateICmpULE(Loaded, Inc);
+    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
   case AtomicRMWInst::FAdd:
-    Res = Builder.CreateFAdd(Orig, Val);
-    break;
+    return Builder.CreateFAdd(Loaded, Inc, "new");
   case AtomicRMWInst::FSub:
-    Res = Builder.CreateFSub(Orig, Val);
-    break;
-  }
+    return Builder.CreateFSub(Loaded, Inc, "new");
+  default:
+    llvm_unreachable("Unknown atomic op");
+  }
+
+bool llvm::lowerAtomicRMWInst(AtomicRMWInst *RMWI) {
+  IRBuilder<> Builder(RMWI);
+  Value *Ptr = RMWI->getPointerOperand();
+  Value *Val = RMWI->getValOperand();
+
+  LoadInst *Orig = Builder.CreateLoad(Val->getType(), Ptr);
+  Value *Res = buildAtomicRMWValue(RMWI->getOperation(), Builder, Orig, Val);
   Builder.CreateStore(Res, Ptr);
   RMWI->replaceAllUsesWith(Orig);
   RMWI->eraseFromParent();
   return true;
 }