Skip to content

Conversation

@anchuraj
Copy link
Contributor

@anchuraj anchuraj commented May 13, 2025

adds function attribute "amdgpu-unsafe-fp-atomics". This allows amdgpu backend to use
unsafe fp atomic instructions in these functions.

@anchuraj anchuraj force-pushed the munsafe-atomics-support branch from 61923bd to 6418170 Compare May 14, 2025 04:56
@anchuraj anchuraj changed the title [mlir][llvm] Changes to support munsafe-atomics [mlir][llvm] Changes to support amd gpu unsafe floating point operations. May 14, 2025
@anchuraj anchuraj marked this pull request as ready for review May 14, 2025 17:09
@anchuraj anchuraj requested review from ftynse and gysit May 14, 2025 17:09
@anchuraj anchuraj requested a review from mjklemm May 14, 2025 17:09
@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Anchu Rajendran S (anchuraj)

Changes

adds function attribute "amdgpu-unsafe-fp-atomics". This allows amdgpu backend to use
unsafe fp atomic instructions in these functions.


Full diff: https://github.com/llvm/llvm-project/pull/139801.diff

6 Files Affected:

  • (modified) mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td (+1)
  • (modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+5)
  • (modified) mlir/lib/Target/LLVMIR/ModuleTranslation.cpp (+4)
  • (modified) mlir/test/Dialect/LLVMIR/func.mlir (+6)
  • (modified) mlir/test/Target/LLVMIR/Import/function-attributes.ll (+6)
  • (modified) mlir/test/Target/LLVMIR/llvmir.mlir (+9)
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index f19f9d5a3083c..93f754607fdac 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1899,6 +1899,7 @@ def LLVM_LLVMFuncOp : LLVM_Op<"func", [
     OptionalAttr<BoolAttr>:$no_nans_fp_math,
     OptionalAttr<BoolAttr>:$approx_func_fp_math,
     OptionalAttr<BoolAttr>:$no_signed_zeros_fp_math,
+    OptionalAttr<BoolAttr>:$amdgpu_unsafe_fp_atomics,
     OptionalAttr<StrAttr>:$denormal_fp_math,
     OptionalAttr<StrAttr>:$denormal_fp_math_f32,
     OptionalAttr<StrAttr>:$fp_contract,
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index 0b77a3d23d392..1b18e2e5c3a89 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -2140,6 +2140,7 @@ static constexpr std::array kExplicitAttributes{
     StringLiteral("aarch64_pstate_sm_body"),
     StringLiteral("aarch64_pstate_sm_compatible"),
     StringLiteral("aarch64_pstate_sm_enabled"),
+    StringLiteral("amdgpu-unsafe-fp-atomics"),
     StringLiteral("alwaysinline"),
     StringLiteral("approx-func-fp-math"),
     StringLiteral("convergent"),
@@ -2318,6 +2319,10 @@ void ModuleImport::processFunctionAttributes(llvm::Function *func,
       attr.isStringAttribute())
     funcOp.setNoSignedZerosFpMath(attr.getValueAsBool());
 
+  if (llvm::Attribute attr = func->getFnAttribute("amdgpu-unsafe-fp-atomics");
+      attr.isStringAttribute())
+    funcOp.setAmdgpuUnsafeFpAtomics(attr.getValueAsBool());
+
   if (llvm::Attribute attr = func->getFnAttribute("denormal-fp-math");
       attr.isStringAttribute())
     funcOp.setDenormalFpMathAttr(
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index ad4f65da9d7f5..bf8d9f74318b3 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -1496,6 +1496,10 @@ LogicalResult ModuleTranslation::convertOneFunction(LLVMFuncOp func) {
   else if (func.getArmPreservesZa())
     llvmFunc->addFnAttr("aarch64_preserves_za");
 
+  if (auto amdgpuUnsafeFpAtomics = func.getAmdgpuUnsafeFpAtomics())
+    llvmFunc->addFnAttr("amdgpu-unsafe-fp-atomics",
+                        llvm::toStringRef(*amdgpuUnsafeFpAtomics));
+
   if (auto targetCpu = func.getTargetCpu())
     llvmFunc->addFnAttr("target-cpu", *targetCpu);
 
diff --git a/mlir/test/Dialect/LLVMIR/func.mlir b/mlir/test/Dialect/LLVMIR/func.mlir
index a168cebff019b..90a24b5f4a40e 100644
--- a/mlir/test/Dialect/LLVMIR/func.mlir
+++ b/mlir/test/Dialect/LLVMIR/func.mlir
@@ -236,6 +236,12 @@ module {
     llvm.return
   }
 
+  llvm.func @amdgpu_fp_unsafe_atomics() attributes {amdgpu_fp_unsafe_atomics = true} {
+    // CHECK: @amdgpu_fp_unsafe_atomics
+    // CHECK-SAME: attributes {amdgpu_fp_unsafe_atomics = true}
+    llvm.return
+  }
+
   // CHECK: llvm.comdat @__llvm_comdat
   llvm.comdat @__llvm_comdat {
     // CHECK: llvm.comdat_selector @any any
diff --git a/mlir/test/Target/LLVMIR/Import/function-attributes.ll b/mlir/test/Target/LLVMIR/Import/function-attributes.ll
index 0b13645853cea..e9f8b707b85a7 100644
--- a/mlir/test/Target/LLVMIR/Import/function-attributes.ll
+++ b/mlir/test/Target/LLVMIR/Import/function-attributes.ll
@@ -267,6 +267,12 @@ define void @arm_preserves_za_func() "aarch64_preserves_za" {
   ret void
 }
 
+; // -----
+
+; CHECK-LABEL: @func_attr_amdgpu_unsafe_fp_atomics_true
+; CHECK-SAME: attributes {amdgpu_unsafe_fp_atomics = true}
+declare void @func_attr_amdgpu_unsafe_fp_atomics_true() "amdgpu-unsafe-fp-atomics"="true"
+
 // -----
 
 ; CHECK-LABEL: @section_func
diff --git a/mlir/test/Target/LLVMIR/llvmir.mlir b/mlir/test/Target/LLVMIR/llvmir.mlir
index 9852c4051f0d0..ade6cbf505e9e 100644
--- a/mlir/test/Target/LLVMIR/llvmir.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir.mlir
@@ -2474,6 +2474,15 @@ llvm.func @preserves_za_func() attributes {arm_preserves_za} {
 
 // -----
 
+// CHECK-LABEL: @amdgpu_unsafe_fp_atomics_func
+// CHECK-SAME: #[[ATTR:[0-9]*]]
+llvm.func @amdgpu_unsafe_fp_atomics_func() attributes {amdgpu_unsafe_fp_atomics = true} {
+  llvm.return
+}
+// CHECK: #[[ATTR]] = { "amdgpu-unsafe-fp-atomics"="true" }
+
+// -----
+
 //
 // frame pointer attribute.
 //

@anchuraj anchuraj requested a review from skatrak May 14, 2025 17:09
@krzysz00
Copy link
Contributor

I could've sworn we explicitly moved away from having this be a function attribute?

@anchuraj anchuraj closed this May 14, 2025
@anchuraj
Copy link
Contributor Author

I could've sworn we explicitly moved away from having this be a function attribute?

Thank you for the input. Noticed it just now. Will raise a new PR for the support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants