[amdgpu] Atomic should be source of divergence.

[hliao]

Update summary.

[arsenm]

llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll

27

Can you add exhaustive tests for all of these instructions (plus make sure llvm.amdgcn.atomic* work)

[hliao]

Update tests with more atomic ops.

[hliao]

llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll

27

done, please check whether there are still missing ones.

[kzhuravl]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11841

always

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11808

I didn't know about this one

[hliao]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11808

Only AArch64 uses that. Just add that conservatively.

[foad]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11802

Would it make sense to handle these generic nodes in a new TargetLowering::isSDNodeSourceOfDivergence so that we don't have to maintain this list in every target that cares about divergence?

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11802

We don't really have a generic definition of divergence. For CPU targets the answer would still be no

[foad]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11802

I was hoping CPU targets wouldn't care what we put in TargetLowering::isSDNodeSourceOfDivergence because they never consume the computed divergence information.

[foad]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11802

Another idea to avoid maintaining this list of opcodes: could you return true for any MemSDNode that isAtomic() ?

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

11802

That should work

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

bool SITargetLowering::isSDNodeSourceOfDivergence(
   case ISD::CALLSEQ_END:
     return true;
   case ISD::INTRINSIC_WO_CHAIN:
     return AMDGPU::isIntrinsicSourceOfDivergence(
         cast<ConstantSDNode>(N->getOperand(0))->getZExtValue());
   case ISD::INTRINSIC_W_CHAIN:
     return AMDGPU::isIntrinsicSourceOfDivergence(
         cast<ConstantSDNode>(N->getOperand(1))->getZExtValue());
+  case ISD::ATOMIC_SWAP:
+  case ISD::ATOMIC_LOAD_ADD:
+  case ISD::ATOMIC_LOAD_SUB:
+  case ISD::ATOMIC_LOAD_AND:
+  case ISD::ATOMIC_LOAD_CLR:
+  case ISD::ATOMIC_LOAD_OR:
+  case ISD::ATOMIC_LOAD_XOR:
+  case ISD::ATOMIC_LOAD_NAND:
+  case ISD::ATOMIC_LOAD_MIN:
+  case ISD::ATOMIC_LOAD_MAX:
+  case ISD::ATOMIC_LOAD_UMIN:
+  case ISD::ATOMIC_LOAD_UMAX:
+  case ISD::ATOMIC_LOAD_FADD:
+  case ISD::ATOMIC_LOAD_FSUB:
+    // TODO: May need to check operand values to determine divergence. Besides
+    // some atomic ops like CLR are alway divergent. The divergence of some
+    // atomic ops is value dependant. For instance, atomic<op> p, v is always
+    // uniform if p is uniform and v is 0 for ADD/SUB/XOR/FADD/FSUB. But, for
+    // other non-zero v, those atomic ops are always divergent for non-zero v
+    // no matter the divergence of p or v. The remaining atomic ops only depend
+    // on divergences of their operands. For example, atomic<op> p, v is always
+    // uniform if p and v are uniform.
+    return true;
   }
   return false;
 }
 
 bool SITargetLowering::denormalsEnabledForType(const SelectionDAG &DAG,
                                                EVT VT) const {
   switch (VT.getScalarType().getSimpleVT().SimpleTy) {
   case MVT::f32:
     return hasFP32Denormals(DAG.getMachineFunction());
   case MVT::f64:
   case MVT::f16:
     return hasFP64FP16Denormals(DAG.getMachineFunction());
   default:
     return false;
   }
 }
 
 bool SITargetLowering::isKnownNeverNaNForTargetNode(SDValue Op,
                                                     const SelectionDAG &DAG,
                                                     bool SNaN,
                                                     unsigned Depth) const {
   if (Op.getOpcode() == AMDGPUISD::CLAMP) {
     const MachineFunction &MF = DAG.getMachineFunction();
     const SIMachineFunctionInfo *Info = MF.getInfo<SIMachineFunctionInfo>();
 

llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -march=amdgcn -mcpu=gfx906 -o - %s | FileCheck %s
+
+%S = type <{ float, double }>
+
+; The result of that atomic ops should not be used as a uniform value.
+define protected amdgpu_kernel void @foo(i32 addrspace(1)* %p, %S addrspace(1)* %q) {
+; CHECK-LABEL: foo:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_load_dwordx4 s[0:3], s[0:1], 0x24
+; CHECK-NEXT:    v_mov_b32_e32 v2, 0
+; CHECK-NEXT:    v_mov_b32_e32 v3, 1
+; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    global_atomic_add v2, v2, v3, s[0:1] glc
+; CHECK-NEXT:    v_mov_b32_e32 v0, s2
+; CHECK-NEXT:    v_mov_b32_e32 v1, s3
+; CHECK-NEXT:    s_waitcnt vmcnt(0)
+; CHECK-NEXT:    v_mad_u64_u32 v[0:1], s[0:1], v2, 12, v[0:1]
+; CHECK-NEXT:    v_mov_b32_e32 v2, 1.0
+; CHECK-NEXT:    global_store_dword v[0:1], v2, off
+; CHECK-NEXT:    s_endpgm
+  %n32 = atomicrmw add i32 addrspace(1)* %p, i32 1 monotonic
+  %n64 = zext i32 %n32 to i64
+  %p1 = getelementptr inbounds %S, %S addrspace(1)* %q, i64 %n64, i32 0
+  store float 1.0, float addrspace(1)* %p1
+  ret void
+}