[llvm-dev] RFC: atomic operations on SI+

Tom Stellard via llvm-dev llvm-dev at lists.llvm.org
Mon Mar 28 06:54:06 PDT 2016


On Fri, Mar 25, 2016 at 02:22:11PM -0400, Jan Vesely wrote:
> Hi Tom, Matt,
> 
> I'm working on a project that needs few coherent atomic operations (HSA
> mode: load, store, compare-and-swap) for std::atomic_uint in HCC.
> 
> the attached patch implements atomic compare and swap for SI+
> (untested). I tried to stay within what was available, but there are
> few issues that I was unsure how to address:
> 
> 1.) it currently uses v2i32 for both input and output. This needlessly
> clobbers a register, but I haven't found a way to say 'output = the
> first subreg for input or output = input' in pattern constraints.
> 
> 2.) I think these will need SLC bit set to work correctly on HSA
> targets. I'm not sure what the best way is to do this. I considered:
> * adding it as an operand to the newly introduced node
> (AMDGPUISD::CMP_SWAP), and setting it to 0/1 in Lowering pass. Can this
> be done without changing order of the inputs? slc is always the last
> but the count is variable
> * introducing HSA variants of the instructions with SLC bit set
> * setting the bit in DAG combine (is there a way to know if I'm
> combining atomic op, or do I need to split the SelectMUBUF functions?)
> 
> 3.) depending on 2 atomic load/store can be either (preferably) MUBUF
> load/store with glc and slc set, or hacked to be either swp_noret
> (store) or cmp_swap 0,0 (load).
> 

Have you seen this patch: http://reviews.llvm.org/D17280

-Tom

> thanks,
> -- 
> Jan Vesely <jan.vesely at rutgers.edu>

> From ffd2c28fb68e958682dc60090a2b1fb669897423 Mon Sep 17 00:00:00 2001
> From: Jan Vesely <jan.vesely at rutgers.edu>
> Date: Tue, 22 Mar 2016 13:54:55 -0400
> Subject: [PATCH 1/1] AMDGPU,SI: Implement atomic compare and swap
> 
> ---
>  lib/Target/AMDGPU/AMDGPUISelLowering.cpp |  1 +
>  lib/Target/AMDGPU/AMDGPUISelLowering.h   |  1 +
>  lib/Target/AMDGPU/CIInstructions.td      |  3 +-
>  lib/Target/AMDGPU/SIISelLowering.cpp     | 41 +++++++++++++++
>  lib/Target/AMDGPU/SIISelLowering.h       |  1 +
>  lib/Target/AMDGPU/SIInstrInfo.td         |  9 ++++
>  lib/Target/AMDGPU/SIInstructions.td      |  2 +-
>  test/CodeGen/AMDGPU/global_atomics.ll    | 89 ++++++++++++++++++++++++++++++++
>  8 files changed, 145 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> index 0820898..6c5b1ed 100644
> --- a/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
> @@ -2782,6 +2782,7 @@ const char* AMDGPUTargetLowering::getTargetNodeName(unsigned Opcode) const {
>    NODE_NAME_CASE(INTERP_P2)
>    NODE_NAME_CASE(STORE_MSKOR)
>    NODE_NAME_CASE(TBUFFER_STORE_FORMAT)
> +  NODE_NAME_CASE(CMP_SWAP)
>    case AMDGPUISD::LAST_AMDGPU_ISD_NUMBER: break;
>    }
>    return nullptr;
> diff --git a/lib/Target/AMDGPU/AMDGPUISelLowering.h b/lib/Target/AMDGPU/AMDGPUISelLowering.h
> index 4627f34..fce8e76 100644
> --- a/lib/Target/AMDGPU/AMDGPUISelLowering.h
> +++ b/lib/Target/AMDGPU/AMDGPUISelLowering.h
> @@ -313,6 +313,7 @@ enum NodeType : unsigned {
>    STORE_MSKOR,
>    LOAD_CONSTANT,
>    TBUFFER_STORE_FORMAT,
> +  CMP_SWAP,
>    LAST_AMDGPU_ISD_NUMBER
>  };
>  
> diff --git a/lib/Target/AMDGPU/CIInstructions.td b/lib/Target/AMDGPU/CIInstructions.td
> index 593300f..d99b013 100644
> --- a/lib/Target/AMDGPU/CIInstructions.td
> +++ b/lib/Target/AMDGPU/CIInstructions.td
> @@ -156,7 +156,7 @@ defm FLAT_ATOMIC_SWAP : FLAT_ATOMIC <
>    flat<0x30, 0x40>, "flat_atomic_swap", VGPR_32
>  >;
>  defm FLAT_ATOMIC_CMPSWAP : FLAT_ATOMIC <
> -  flat<0x31, 0x41>, "flat_atomic_cmpswap", VGPR_32, VReg_64
> +  flat<0x31, 0x41>, "flat_atomic_cmpswap", VReg_64
>  >;
>  defm FLAT_ATOMIC_ADD : FLAT_ATOMIC <
>    flat<0x32, 0x42>, "flat_atomic_add", VGPR_32
> @@ -322,6 +322,7 @@ def : FlatAtomicPat <FLAT_ATOMIC_SMIN_RTN, atomic_min_global, i32>;
>  def : FlatAtomicPat <FLAT_ATOMIC_UMIN_RTN, atomic_umin_global, i32>;
>  def : FlatAtomicPat <FLAT_ATOMIC_OR_RTN, atomic_or_global, i32>;
>  def : FlatAtomicPat <FLAT_ATOMIC_SWAP_RTN, atomic_swap_global, i32>;
> +def : FlatAtomicPat <FLAT_ATOMIC_CMPSWAP_RTN, SIcmp_swap, v2i32>;
>  def : FlatAtomicPat <FLAT_ATOMIC_XOR_RTN, atomic_xor_global, i32>;
>  
>  } // End Predicates = [isCIVI]
> diff --git a/lib/Target/AMDGPU/SIISelLowering.cpp b/lib/Target/AMDGPU/SIISelLowering.cpp
> index a8f6ed5..4b6d118 100644
> --- a/lib/Target/AMDGPU/SIISelLowering.cpp
> +++ b/lib/Target/AMDGPU/SIISelLowering.cpp
> @@ -263,6 +263,12 @@ SITargetLowering::SITargetLowering(TargetMachine &TM,
>    setOperationAction(ISD::FDIV, MVT::f32, Custom);
>    setOperationAction(ISD::FDIV, MVT::f64, Custom);
>  
> +  // GCN CMP_SWAP needs input marshalling, and output demarshalling
> +  setOperationAction(ISD::ATOMIC_CMP_SWAP, MVT::i32, Custom);
> +  // We can't return success/failure, only the old value,
> +  // let LLVM add the comparison
> +  setOperationAction(ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS, MVT::i32, Expand);
> +
>    setTargetDAGCombine(ISD::FADD);
>    setTargetDAGCombine(ISD::FSUB);
>    setTargetDAGCombine(ISD::FMINNUM);
> @@ -1168,6 +1174,7 @@ SDValue SITargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
>      SIMachineFunctionInfo *MFI = MF.getInfo<SIMachineFunctionInfo>();
>      return LowerGlobalAddress(MFI, Op, DAG);
>    }
> +  case ISD::ATOMIC_CMP_SWAP: return LowerATOMIC_CMP_SWAP(Op, DAG);
>    case ISD::INTRINSIC_WO_CHAIN: return LowerINTRINSIC_WO_CHAIN(Op, DAG);
>    case ISD::INTRINSIC_VOID: return LowerINTRINSIC_VOID(Op, DAG);
>    }
> @@ -1680,6 +1687,40 @@ SDValue SITargetLowering::LowerINTRINSIC_VOID(SDValue Op,
>    }
>  }
>  
> +SDValue SITargetLowering::LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const
> +{
> +  SDLoc DL(Op);
> +
> +  AtomicSDNode *Swap = cast<AtomicSDNode>(Op);
> +  assert(Swap && Swap->isCompareAndSwap());
> +
> +  EVT MemVT = Swap->getMemoryVT();
> +  EVT ValVT = EVT::getVectorVT(*DAG.getContext(), MemVT, 2);
> +  SDValue Cmp = Op.getOperand(2);
> +  SDValue New = Op.getOperand(3);
> +
> +  // "src comes from the first data-vgpr, cmp from the second."
> +  SDValue Val = DAG.getNode(ISD::BUILD_VECTOR, DL, ValVT, New, Cmp);
> +
> +  SDValue Ops[] = {Swap->getChain(), Swap->getBasePtr(), Val};
> +  ArrayRef<EVT> VTs = { MVT::v2i32, MVT::Other };
> +  SDVTList VTList = { VTs.data(), VTs.size() };
> +
> +  SDValue CmpSwap = DAG.getMemIntrinsicNode(AMDGPUISD::CMP_SWAP, DL, VTList,
> +                                            Ops, MemVT, Swap->getMemOperand());
> +  // TODO: What about ordering? synchscope?
> +
> +  // Extract returned old value
> +  SDValue Zero = DAG.getConstant(0, DL, MVT::i32);
> +  SDValue Old  = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::i32,
> +                             CmpSwap.getValue(0), Zero);
> +
> +  // Merge return value and Chain
> +  SDValue Ret = DAG.getNode(ISD::MERGE_VALUES, DL, Op->getVTList(),
> +                            Old, CmpSwap.getValue(1));
> +  return Ret;
> +}
> +
>  SDValue SITargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const {
>    SDLoc DL(Op);
>    LoadSDNode *Load = cast<LoadSDNode>(Op);
> diff --git a/lib/Target/AMDGPU/SIISelLowering.h b/lib/Target/AMDGPU/SIISelLowering.h
> index 842fbeb..2f87de1 100644
> --- a/lib/Target/AMDGPU/SIISelLowering.h
> +++ b/lib/Target/AMDGPU/SIISelLowering.h
> @@ -29,6 +29,7 @@ class SITargetLowering final : public AMDGPUTargetLowering {
>    SDValue lowerImplicitZextParam(SelectionDAG &DAG, SDValue Op,
>                                   MVT VT, unsigned Offset) const;
>  
> +  SDValue LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const;
>    SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
>    SDValue LowerINTRINSIC_VOID(SDValue Op, SelectionDAG &DAG) const;
>    SDValue LowerFrameIndex(SDValue Op, SelectionDAG &DAG) const;
> diff --git a/lib/Target/AMDGPU/SIInstrInfo.td b/lib/Target/AMDGPU/SIInstrInfo.td
> index 37ee35c..eb0e6ce 100644
> --- a/lib/Target/AMDGPU/SIInstrInfo.td
> +++ b/lib/Target/AMDGPU/SIInstrInfo.td
> @@ -114,6 +114,15 @@ def SItbuffer_store : SDNode<"AMDGPUISD::TBUFFER_STORE_FORMAT",
>    [SDNPMayStore, SDNPMemOperand, SDNPHasChain]
>  >;
>  
> +def SIcmp_swap : SDNode<"AMDGPUISD::CMP_SWAP",
> +  SDTypeProfile<1, 2,
> +    [SDTCisVT<0, v2i32>, // return value (should be i32)
> +     SDTCisVT<1, i64>,   // address
> +     SDTCisVT<2, v2i32>  // src followed by cmp
> +    ]>,
> +    [SDNPMayLoad, SDNPMayStore, SDNPMemOperand, SDNPHasChain]
> +>;
> +
>  def SIload_input : SDNode<"AMDGPUISD::LOAD_INPUT",
>    SDTypeProfile<1, 3, [SDTCisVT<0, v4f32>, SDTCisVT<1, v4i32>, SDTCisVT<2, i16>,
>                         SDTCisVT<3, i32>]>
> diff --git a/lib/Target/AMDGPU/SIInstructions.td b/lib/Target/AMDGPU/SIInstructions.td
> index 2f7f908..77bb8ca 100644
> --- a/lib/Target/AMDGPU/SIInstructions.td
> +++ b/lib/Target/AMDGPU/SIInstructions.td
> @@ -1012,7 +1012,7 @@ defm BUFFER_ATOMIC_SWAP : MUBUF_Atomic <
>    mubuf<0x30, 0x40>, "buffer_atomic_swap", VGPR_32, i32, atomic_swap_global
>  >;
>  defm BUFFER_ATOMIC_CMPSWAP : MUBUF_Atomic <
> -  mubuf<0x31, 0x41>, "buffer_atomic_cmpswap", VReg_64, v2i32, null_frag
> +  mubuf<0x31, 0x41>, "buffer_atomic_cmpswap", VReg_64, v2i32, SIcmp_swap
>  >;
>  defm BUFFER_ATOMIC_ADD : MUBUF_Atomic <
>    mubuf<0x32, 0x42>, "buffer_atomic_add", VGPR_32, i32, atomic_add_global
> diff --git a/test/CodeGen/AMDGPU/global_atomics.ll b/test/CodeGen/AMDGPU/global_atomics.ll
> index a92ee89..340e808 100644
> --- a/test/CodeGen/AMDGPU/global_atomics.ll
> +++ b/test/CodeGen/AMDGPU/global_atomics.ll
> @@ -758,6 +758,95 @@ entry:
>    ret void
>  }
>  
> +; CMP_SWAP
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_offset:
> +; GCN: buffer_atomic_cmpswap v[{{[0-9]+}}:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 offset:16{{$}}
> +define void @atomic_cmpxchg_i32_offset(i32 addrspace(1)* %out, i32 %in, i32 %old) {
> +entry:
> +  %gep = getelementptr i32, i32 addrspace(1)* %out, i32 4
> +  %0  = cmpxchg volatile i32 addrspace(1)* %gep, i32 %old, i32 %in seq_cst seq_cst
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_ret_offset:
> +; GCN: buffer_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]{{:[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 offset:16 glc{{$}}
> +; GCN: buffer_store_dword v[[RET]]
> +define void @atomic_cmpxchg_i32_ret_offset(i32 addrspace(1)* %out, i32 addrspace(1)* %out2, i32 %in, i32 %old) {
> +entry:
> +  %gep = getelementptr i32, i32 addrspace(1)* %out, i32 4
> +  %0  = cmpxchg volatile i32 addrspace(1)* %gep, i32 %old, i32 %in seq_cst seq_cst
> +  %1  = extractvalue { i32, i1 } %0, 0
> +  store i32 %1, i32 addrspace(1)* %out2
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_addr64_offset:
> +; SI: buffer_atomic_cmpswap v[{{[0-9]+\:[0-9]+}}], v[{{[0-9]+}}:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 addr64 offset:16{{$}}
> +define void @atomic_cmpxchg_i32_addr64_offset(i32 addrspace(1)* %out, i32 %in, i64 %index, i32 %old) {
> +entry:
> +  %ptr = getelementptr i32, i32 addrspace(1)* %out, i64 %index
> +  %gep = getelementptr i32, i32 addrspace(1)* %ptr, i32 4
> +  %0   = cmpxchg volatile i32 addrspace(1)* %gep, i32 %old, i32 %in seq_cst seq_cst
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_ret_addr64_offset:
> +; SI: buffer_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]:{{[0-9]+}}], v[{{[0-9]+}}:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 addr64 offset:16 glc{{$}}
> +; VI: flat_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]:{{[0-9]+}}], v[{{[0-9]+:[0-9]+}}], v[{{[0-9]+:[0-9]+}}] glc{{$}}
> +; GCN: buffer_store_dword v[[RET]]
> +define void @atomic_cmpxchg_i32_ret_addr64_offset(i32 addrspace(1)* %out, i32 addrspace(1)* %out2, i32 %in, i64 %index, i32 %old) {
> +entry:
> +  %ptr = getelementptr i32, i32 addrspace(1)* %out, i64 %index
> +  %gep = getelementptr i32, i32 addrspace(1)* %ptr, i32 4
> +  %0   = cmpxchg volatile i32 addrspace(1)* %gep, i32 %old, i32 %in seq_cst seq_cst
> +  %1  = extractvalue { i32, i1 } %0, 0
> +  store i32 %1, i32 addrspace(1)* %out2
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32:
> +; GCN: buffer_atomic_cmpswap v[{{[0-9]+:[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0{{$}}
> +define void @atomic_cmpxchg_i32(i32 addrspace(1)* %out, i32 %in, i32 %old) {
> +entry:
> +  %0  = cmpxchg volatile i32 addrspace(1)* %out, i32 %old, i32 %in seq_cst seq_cst
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_ret:
> +; GCN: buffer_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 glc
> +; GCN: buffer_store_dword v[[RET]]
> +define void @atomic_cmpxchg_i32_ret(i32 addrspace(1)* %out, i32 addrspace(1)* %out2, i32 %in, i32 %old) {
> +entry:
> +  %0  = cmpxchg volatile i32 addrspace(1)* %out, i32 %old, i32 %in seq_cst seq_cst
> +  %1  = extractvalue { i32, i1 } %0, 0
> +  store i32 %1, i32 addrspace(1)* %out2
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_addr64:
> +; SI: buffer_atomic_cmpswap v[{{[0-9]+:[0-9]+}}], v[{{[0-9]+}}:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 addr64{{$}}
> +; VI: flat_atomic_cmpswap v[{{[0-9]+:[0-9]+}}], v[{{[0-9]+:[0-9]+}}]{{$}}
> +define void @atomic_cmpxchg_i32_addr64(i32 addrspace(1)* %out, i32 %in, i64 %index, i32 %old) {
> +entry:
> +  %ptr = getelementptr i32, i32 addrspace(1)* %out, i64 %index
> +  %0  = cmpxchg volatile i32 addrspace(1)* %ptr, i32 %old, i32 %in seq_cst seq_cst
> +  ret void
> +}
> +
> +; FUNC-LABEL: {{^}}atomic_cmpxchg_i32_ret_addr64:
> +; SI: buffer_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]:{{[0-9]+}}], v[{{[0-9]+}}:{{[0-9]+}}], s[{{[0-9]+}}:{{[0-9]+}}], 0 addr64 glc{{$}}
> +; VI: flat_atomic_cmpswap v{{\[}}[[RET:[0-9]+]]:{{[0-9]+}}], v[{{[0-9]+:[0-9]+}}], v[{{[0-9]+:[0-9]+}}] glc{{$}}
> +; GCN: buffer_store_dword v[[RET]]
> +define void @atomic_cmpxchg_i32_ret_addr64(i32 addrspace(1)* %out, i32 addrspace(1)* %out2, i32 %in, i64 %index, i32 %old) {
> +entry:
> +  %ptr = getelementptr i32, i32 addrspace(1)* %out, i64 %index
> +  %0  = cmpxchg volatile i32 addrspace(1)* %ptr, i32 %old, i32 %in seq_cst seq_cst
> +  %1  = extractvalue { i32, i1 } %0, 0
> +  store i32 %1, i32 addrspace(1)* %out2
> +  ret void
> +}
> +
>  ; FUNC-LABEL: {{^}}atomic_xor_i32_offset:
>  ; GCN: buffer_atomic_xor v{{[0-9]+}}, s[{{[0-9]+}}:{{[0-9]+}}], 0 offset:16{{$}}
>  define void @atomic_xor_i32_offset(i32 addrspace(1)* %out, i32 %in) {
> -- 
> 2.5.5
> 





More information about the llvm-dev mailing list