[Remarks] [AMDGPU] Emit optimization remarks for atomics generating hardware instructions

[gandhi21299]

Summary

• produce remarks when atomic instructions are expanded into hardware instructions in SIISelLowering.cpp
• an OpenCL test containing both IR-level and ISA level checks
• currently only support for atomic fadd

[rampitec]

• Add [AMDGPU] to the title.
• Rebase on top of D106891.
• Add tests to atomics-remarks-gfx90a.ll as well, including LDS with matching and non-matching rounding mode.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12192

This is safe.

12201

This is safe if fpModeMatchesGlobalFPAtomicMode(RMW) returned true and unsafe if not.

[gandhi21299]

• added more tests
• addressed feedback from reviewer

[gandhi21299]

• corrected an argument in AtomicExpand pass

[rampitec]

clang/test/CodeGenOpenCL/atomics-cas-remarks-gfx90a.cl

9

You are compiling 2 functions with 2 different sets of options. Essentially it is unclear what are you checking because either half skips half of the remarks. Either compile a single function differently or make 2 different tests.

13

This line does not have -munsafe-fp-atomics option...

52

... therefor these checks must fail.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12194

This is wrong. Condition is inverted and essentially tests should fail. Make sure you can pass testing before posting a diff.

llvm/test/CodeGen/AMDGPU/atomics-cas-remarks-gfx90a.ll

108

Does it print a function name before the diagnostics? Label checks would be useful.

[gandhi21299]

clang/test/CodeGenOpenCL/atomics-cas-remarks-gfx90a.cl

9

I will create 2 seperate tests

llvm/test/CodeGen/AMDGPU/atomics-cas-remarks-gfx90a.ll

108

Nope, it does not.

[gandhi21299]

• split the OpenCL test into two for brevity
• fixed a mistake in SIISelLowering as pointed out by reviewer
• added the missing -munsafe-fp-atomics flag

[gandhi21299]

ping

[rampitec]

clang/test/CodeGenOpenCL/atomics-cas-remarks-gfx90a.cl

9

You do not need to change this file anymore.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12122

It does not need to me a method, just a static function or better even lamdba.

12194

Unresolved.

llvm/test/CodeGen/AMDGPU/atomics-cas-remarks-gfx90a.ll

108

That's pity. Then this file need to be split too.

[gandhi21299]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12194

Remarks are produced if fpModeMatchesGlobalFPAtomicMode(RMW) == false

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12194

But you have changed what function was doing. It was returning CmpXChg.

[gandhi21299]

• requested changes from reviewer

[gandhi21299]

• corrected AtomicExpansionKind return value in SIISelLowering as pointed out by reviewer
• eliminated previous function declaration which is not defined/used anymore

[rampitec]

Logic is still wrong.

[gandhi21299]

@rampitec Which part of the logic is wrong?

[rampitec]

In D108150#2950458, @gandhi21299 wrote:

@rampitec Which part of the logic is wrong?

Still the same around LDS.

[gandhi21299]

My understanding is that since we are reporting unsafe expansion into hw instructions, fpModeMatchesGlobalFPAtomicMode(RMW) must be false to match the logic.

[rampitec]

In D108150#2950479, @gandhi21299 wrote:

My understanding is that since we are reporting unsafe expansion into hw instructions, fpModeMatchesGlobalFPAtomicMode(RMW) must be false to match the logic.

Please run check-llvm before updating the patch.

[gandhi21299]

• corrected logic for ORE in SIISelLowering.cpp

[gandhi21299]

ping

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

12202

if (fpModeMatchesGlobalFPAtomicMode(RMW))
  return AtomicExpansionKind::None;

return RMW->getFunction()
                   ->getFnAttribute("amdgpu-unsafe-fp-atomics")
                   .getValueAsString() == "true"
           ? ReportUnsafeHWInst(AtomicExpansionKind::None)
           : AtomicExpansionKind::CmpXChg;

[gandhi21299]

• code refactor

[gandhi21299]

Thanks for the review, I will merge this in as soon as the CI passes.

clang/test/CodeGenOpenCL/atomics-cas-remarks-gfx90a.cl

 // RUN: %clang_cc1 %s -cl-std=CL2.0 -O0 -triple=amdgcn-amd-amdhsa -target-cpu gfx90a \
 // RUN:     -Rpass=atomic-expand -S -o - 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=REMARK
 
 // RUN: %clang_cc1 %s -cl-std=CL2.0 -O0 -triple=amdgcn-amd-amdhsa -target-cpu gfx90a \
 // RUN:     -Rpass=atomic-expand -S -emit-llvm -o - 2>&1 | \
 // RUN:     FileCheck %s --check-prefix=GFX90A-CAS
 
 // REQUIRES: amdgpu-registered-target
 
 typedef enum memory_order {
   memory_order_relaxed = __ATOMIC_RELAXED,
   memory_order_acquire = __ATOMIC_ACQUIRE,
   memory_order_release = __ATOMIC_RELEASE,
   memory_order_acq_rel = __ATOMIC_ACQ_REL,
   memory_order_seq_cst = __ATOMIC_SEQ_CST
 } memory_order;
 
 typedef enum memory_scope {
   memory_scope_work_item = __OPENCL_MEMORY_SCOPE_WORK_ITEM,
   memory_scope_work_group = __OPENCL_MEMORY_SCOPE_WORK_GROUP,

 // GFX90A-CAS: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("agent-one-as") monotonic
 // GFX90A-CAS: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("one-as") monotonic
 // GFX90A-CAS: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("wavefront-one-as") monotonic
 float atomic_cas(__global atomic_float *d, float a) {
   float ret1 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_work_group);
   float ret2 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_device);
   float ret3 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_all_svm_devices);
   float ret4 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_sub_group);
-}
+}
 No newline at end of file

clang/test/CodeGenOpenCL/atomics-unsafe-hw-remarks-gfx90a.cl

+// RUN: %clang_cc1 -cl-std=CL2.0 -O0 -triple=amdgcn-amd-amdhsa -target-cpu gfx90a \
+// RUN:     -Rpass=si-lower -munsafe-fp-atomics %s -S -emit-llvm -o - 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=GFX90A-HW
+
+// RUN: %clang_cc1 -cl-std=CL2.0 -O0 -triple=amdgcn-amd-amdhsa -target-cpu gfx90a \
+// RUN:     -Rpass=si-lower -munsafe-fp-atomics %s -S -o - 2>&1 | \
+// RUN:     FileCheck %s --check-prefix=GFX90A-HW-REMARK
+
+
+// REQUIRES: amdgpu-registered-target
+
+typedef enum memory_order {
+  memory_order_relaxed = __ATOMIC_RELAXED,
+  memory_order_acquire = __ATOMIC_ACQUIRE,
+  memory_order_release = __ATOMIC_RELEASE,
+  memory_order_acq_rel = __ATOMIC_ACQ_REL,
+  memory_order_seq_cst = __ATOMIC_SEQ_CST
+} memory_order;
+
+typedef enum memory_scope {
+  memory_scope_work_item = __OPENCL_MEMORY_SCOPE_WORK_ITEM,
+  memory_scope_work_group = __OPENCL_MEMORY_SCOPE_WORK_GROUP,
+  memory_scope_device = __OPENCL_MEMORY_SCOPE_DEVICE,
+  memory_scope_all_svm_devices = __OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES,
+#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups)
+  memory_scope_sub_group = __OPENCL_MEMORY_SCOPE_SUB_GROUP
+#endif
+} memory_scope;
+
+// GFX90A-HW-REMARK: Hardware instruction generated for atomic fadd operation at memory scope workgroup-one-as due to an unsafe request. [-Rpass=si-lower]
+// GFX90A-HW-REMARK: Hardware instruction generated for atomic fadd operation at memory scope agent-one-as due to an unsafe request. [-Rpass=si-lower]
+// GFX90A-HW-REMARK: Hardware instruction generated for atomic fadd operation at memory scope wavefront-one-as due to an unsafe request. [-Rpass=si-lower]
+// GFX90A-HW-REMARK: global_atomic_add_f32 v0, v[0:1], v2, off glc
+// GFX90A-HW-REMARK: global_atomic_add_f32 v0, v[0:1], v2, off glc
+// GFX90A-HW-REMARK: global_atomic_add_f32 v0, v[0:1], v2, off glc
+// GFX90A-HW-LABEL: @atomic_unsafe_hw
+// GFX90A-HW:   atomicrmw fadd float addrspace(1)* %{{.*}}, float %{{.*}} syncscope("workgroup-one-as") monotonic, align 4
+// GFX90A-HW:   atomicrmw fadd float addrspace(1)* %{{.*}}, float %{{.*}} syncscope("agent-one-as") monotonic, align 4
+// GFX90A-HW:   atomicrmw fadd float addrspace(1)* %{{.*}}, float %{{.*}} syncscope("wavefront-one-as") monotonic, align 4
+void atomic_unsafe_hw(__global atomic_float *d, float a) {
+  float ret1 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_work_group);
+  float ret2 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_device);
+  float ret3 = __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_sub_group);
+}

llvm/lib/CodeGen/AtomicExpandPass.cpp

case TargetLoweringBase::AtomicExpansionKind::CmpXChg: {
     } else {
       SmallVector<StringRef> SSNs;
       Ctx.getSyncScopeNames(SSNs);
       auto MemScope = SSNs[AI->getSyncScopeID()].empty()
                           ? "system"
                           : SSNs[AI->getSyncScopeID()];
       OptimizationRemarkEmitter ORE(AI->getFunction());
       ORE.emit([&]() {
-        return OptimizationRemark(DEBUG_TYPE, "Passed", AI->getFunction())
+        return OptimizationRemark(DEBUG_TYPE, "Passed", AI)
                << "A compare and swap loop was generated for an atomic "
                << AI->getOperationName(AI->getOperation()) << " operation at "
                << MemScope << " memory scope";
       });
       expandAtomicRMWToCmpXchg(AI, createCmpXchgInstFun);
     }
     return true;
   }

llvm/lib/Target/AMDGPU/SIISelLowering.h

unsigned getNumRegistersForCallingConv(LLVMContext &Context,
                                          CallingConv::ID CC,
                                          EVT VT) const override;
 
   unsigned getVectorTypeBreakdownForCallingConv(
     LLVMContext &Context, CallingConv::ID CC, EVT VT, EVT &IntermediateVT,
     unsigned &NumIntermediates, MVT &RegisterVT) const override;
 
 private:
+  TargetLowering::AtomicExpansionKind
+  reportUnsafeHWInst(AtomicRMWInst *RMW,
+                     TargetLowering::AtomicExpansionKind Kind) const;
   SDValue lowerKernArgParameterPtr(SelectionDAG &DAG, const SDLoc &SL,
                                    SDValue Chain, uint64_t Offset) const;
   SDValue getImplicitArgPtr(SelectionDAG &DAG, const SDLoc &SL) const;
   SDValue lowerKernargMemParameter(SelectionDAG &DAG, EVT VT, EVT MemVT,
                                    const SDLoc &SL, SDValue Chain,
                                    uint64_t Offset, Align Alignment,
                                    bool Signed,
                                    const ISD::InputArg *Arg = nullptr) const;

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

 #include "SIISelLowering.h"
 #include "AMDGPU.h"
 #include "AMDGPUInstrInfo.h"
 #include "AMDGPUTargetMachine.h"
 #include "SIMachineFunctionInfo.h"
 #include "SIRegisterInfo.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/LegacyDivergenceAnalysis.h"
+#include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/GlobalISel/GISelKnownBits.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/IntrinsicInst.h"

 static bool fpModeMatchesGlobalFPAtomicMode(const AtomicRMWInst *RMW) {
   const fltSemantics &Flt = RMW->getType()->getScalarType()->getFltSemantics();
   auto DenormMode = RMW->getParent()->getParent()->getDenormalMode(Flt);
   if (&Flt == &APFloat::IEEEsingle())
     return DenormMode == DenormalMode::getPreserveSign();
   return DenormMode == DenormalMode::getIEEE();
 }
 
 TargetLowering::AtomicExpansionKind
 SITargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *RMW) const {
+
+  auto ReportUnsafeHWInst = [&](TargetLowering::AtomicExpansionKind Kind) {
+    OptimizationRemarkEmitter ORE(RMW->getFunction());
+    LLVMContext &Ctx = RMW->getFunction()->getContext();
+    SmallVector<StringRef> SSNs;
+    Ctx.getSyncScopeNames(SSNs);
+    auto MemScope = SSNs[RMW->getSyncScopeID()].empty()
+                        ? "system"
+                        : SSNs[RMW->getSyncScopeID()];
+    ORE.emit([&]() {
+      return OptimizationRemark(DEBUG_TYPE, "Passed", RMW)
+             << "Hardware instruction generated for atomic "
+             << RMW->getOperationName(RMW->getOperation())
+             << " operation at memory scope " << MemScope
+             << " due to an unsafe request.";
+    });
+    return Kind;
+  };
+
   switch (RMW->getOperation()) {
   case AtomicRMWInst::FAdd: {
     Type *Ty = RMW->getType();
 
     // We don't have a way to support 16-bit atomics now, so just leave them
     // as-is.
     if (Ty->isHalfTy())
       return AtomicExpansionKind::None;
if ((AS == AMDGPUAS::GLOBAL_ADDRESS || AS == AMDGPUAS::FLAT_ADDRESS) &&
         if (Ty->isFloatTy() && AS == AMDGPUAS::FLAT_ADDRESS)
           return AtomicExpansionKind::CmpXChg;
 
         auto SSID = RMW->getSyncScopeID();
         if (SSID == SyncScope::System ||
             SSID == RMW->getContext().getOrInsertSyncScopeID("one-as"))
           return AtomicExpansionKind::CmpXChg;
 
-        return AtomicExpansionKind::None;
+        return ReportUnsafeHWInst(AtomicExpansionKind::None);
       }
 
       if (AS == AMDGPUAS::FLAT_ADDRESS)
         return AtomicExpansionKind::CmpXChg;
 
-      return RMW->use_empty() ? AtomicExpansionKind::None
+      return RMW->use_empty() ? ReportUnsafeHWInst(AtomicExpansionKind::None)
                               : AtomicExpansionKind::CmpXChg;
     }
 
     // DS FP atomics do repect the denormal mode, but the rounding mode is fixed
     // to round-to-nearest-even.
     // The only exception is DS_ADD_F64 which never flushes regardless of mode.
     if (AS == AMDGPUAS::LOCAL_ADDRESS && Subtarget->hasLDSFPAtomics()) {
       if (!Ty->isDoubleTy())
         return AtomicExpansionKind::None;
 
+      if (!fpModeMatchesGlobalFPAtomicMode(RMW))
+        ReportUnsafeHWInst(AtomicExpansionKind::None);
+
       return (fpModeMatchesGlobalFPAtomicMode(RMW) ||
               RMW->getFunction()
                       ->getFnAttribute("amdgpu-unsafe-fp-atomics")
                       .getValueAsString() == "true")
                  ? AtomicExpansionKind::None
                  : AtomicExpansionKind::CmpXChg;
     }
 
     return AtomicExpansionKind::CmpXChg;
   }
   default:
     break;
   }
 

llvm/test/CodeGen/AMDGPU/atomics-hw-remarks-gfx90a.ll

+; RUN: llc -march=amdgcn -mcpu=gfx90a -verify-machineinstrs --pass-remarks=si-lower \
+; RUN:      %s -o - 2>&1 | FileCheck %s --check-prefix=GFX90A-HW
+
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope system due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope agent due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope workgroup due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope wavefront due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope singlethread due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope agent-one-as due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope workgroup-one-as due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope wavefront-one-as due to an unsafe request.
+; GFX90A-HW: Hardware instruction generated for atomic fadd operation at memory scope singlethread-one-as due to an unsafe request.
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw:
+; GFX90A-HW:    ds_add_f64 v2, v[0:1]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw(double addrspace(3)* %ptr) #0 {
+main_body:
+  %ret = atomicrmw fadd double addrspace(3)* %ptr, double 4.0 seq_cst
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_agent:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_agent(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("agent") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_wg:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_wg(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("workgroup") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_wavefront:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_wavefront(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("wavefront") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_single_thread:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_single_thread(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("singlethread") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_aoa:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_aoa(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("agent-one-as") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_wgoa:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_wgoa(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("workgroup-one-as") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_wfoa:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_wfoa(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("wavefront-one-as") monotonic, align 4
+  ret void
+}
+
+; GFX90A-HW-LABEL: atomic_add_unsafe_hw_stoa:
+; GFX90A-HW:    global_atomic_add_f32 v0, v1, s[2:3]
+; GFX90A-HW:    s_endpgm
+define amdgpu_kernel void @atomic_add_unsafe_hw_stoa(float addrspace(1)* %ptr, float %val) #0 {
+main_body:
+  %ret = atomicrmw fadd float addrspace(1)* %ptr, float %val syncscope("singlethread-one-as") monotonic, align 4
+  ret void
+}
+
+attributes #0 = { "denormal-fp-math"="preserve-sign,preserve-sign" "amdgpu-unsafe-fp-atomics"="true" }