[clang] Fix invalid CodeGen for atomic fetch min/max on floating point values

[Lancern]

CodeGen for the builtin function __atomic_min_fetch and __atomic_max_fetch would emit an atomicrmw instruction followed by an icmp instruction on integral arguments. However, for floating point arguments, CodeGen keeps emitting icmp instruction instead of fcmp instruction.

This patch fixes the issue.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Sirui Mu (Lancern)

Changes

CodeGen for the builtin function __atomic_min_fetch and __atomic_max_fetch would emit an atomicrmw instruction followed by an icmp instruction on integral arguments. However, for floating point arguments, CodeGen keeps emitting icmp instruction instead of fcmp instruction.

This patch fixes the issue.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGAtomic.cpp (+13-10)
• (modified) clang/test/CodeGen/Atomics.c (+17)

diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index 4a3446abcc78f..0a6419476c338 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -503,24 +503,27 @@ static void emitAtomicCmpXchgFailureSet(CodeGenFunction &CGF, AtomicExpr *E,
 /// Duplicate the atomic min/max operation in conventional IR for the builtin
 /// variants that return the new rather than the original value.
 static llvm::Value *EmitPostAtomicMinMax(CGBuilderTy &Builder,
-                                         AtomicExpr::AtomicOp Op,
-                                         bool IsSigned,
-                                         llvm::Value *OldVal,
+                                         AtomicExpr::AtomicOp Op, bool IsInt,
+                                         bool IsSigned, llvm::Value *OldVal,
                                          llvm::Value *RHS) {
-  llvm::CmpInst::Predicate Pred;
+  llvm::CmpInst::Predicate ICmpPred;
+  llvm::FCmpInst::Predicate FCmpPred;
   switch (Op) {
   default:
     llvm_unreachable("Unexpected min/max operation");
   case AtomicExpr::AO__atomic_max_fetch:
   case AtomicExpr::AO__scoped_atomic_max_fetch:
-    Pred = IsSigned ? llvm::CmpInst::ICMP_SGT : llvm::CmpInst::ICMP_UGT;
+    ICmpPred = IsSigned ? llvm::CmpInst::ICMP_SGT : llvm::CmpInst::ICMP_UGT;
+    FCmpPred = llvm::FCmpInst::FCMP_UGT;
     break;
   case AtomicExpr::AO__atomic_min_fetch:
   case AtomicExpr::AO__scoped_atomic_min_fetch:
-    Pred = IsSigned ? llvm::CmpInst::ICMP_SLT : llvm::CmpInst::ICMP_ULT;
+    ICmpPred = IsSigned ? llvm::CmpInst::ICMP_SLT : llvm::CmpInst::ICMP_ULT;
+    FCmpPred = llvm::FCmpInst::FCMP_ULT;
     break;
   }
-  llvm::Value *Cmp = Builder.CreateICmp(Pred, OldVal, RHS, "tst");
+  llvm::Value *Cmp = IsInt ? Builder.CreateICmp(ICmpPred, OldVal, RHS, "tst")
+                           : Builder.CreateFCmp(FCmpPred, OldVal, RHS, "tst");
   return Builder.CreateSelect(Cmp, OldVal, RHS, "newval");
 }
 
@@ -760,9 +763,9 @@ static void EmitAtomicOp(CodeGenFunction &CGF, AtomicExpr *E, Address Dest,
   // determine the value which was written.
   llvm::Value *Result = RMWI;
   if (PostOpMinMax)
-    Result = EmitPostAtomicMinMax(CGF.Builder, E->getOp(),
-                                  E->getValueType()->isSignedIntegerType(),
-                                  RMWI, LoadVal1);
+    Result = EmitPostAtomicMinMax(
+        CGF.Builder, E->getOp(), E->getValueType()->isIntegerType(),
+        E->getValueType()->isSignedIntegerType(), RMWI, LoadVal1);
   else if (PostOp)
     Result = CGF.Builder.CreateBinOp((llvm::Instruction::BinaryOps)PostOp, RMWI,
                                      LoadVal1);
diff --git a/clang/test/CodeGen/Atomics.c b/clang/test/CodeGen/Atomics.c
index c44f45efacab0..dd5936cf48b46 100644
--- a/clang/test/CodeGen/Atomics.c
+++ b/clang/test/CodeGen/Atomics.c
@@ -12,6 +12,7 @@ signed long long sll;
 unsigned long long ull;
 __int128 s128;
 unsigned  __int128 u128;
+float f;
 
 void test_op_ignore (void) // CHECK-LABEL: define{{.*}} void @test_op_ignore
 {
@@ -318,3 +319,19 @@ void test_atomic(void) {
   ui = __atomic_fetch_max(&ui, 5, __ATOMIC_ACQUIRE); // CHECK: atomicrmw umax {{.*}} acquire, align 4
   si = __atomic_fetch_max(&si, 5, __ATOMIC_RELEASE); // CHECK: atomicrmw max {{.*}} release, align 4
 }
+
+void test_atomic_minmax_fp(void) {
+  f = __atomic_fetch_min(&f, 5.0f, __ATOMIC_RELAXED);
+  // CHECK: atomicrmw fmin {{.*}} monotonic, align 4
+
+  f = __atomic_fetch_max(&f, 5.0, __ATOMIC_RELAXED);
+  // CHECK: atomicrmw fmax {{.*}} monotonic, align 4
+
+  f = __atomic_min_fetch(&f, 5.0, __ATOMIC_RELAXED);
+  // CHECK:      atomicrmw fmin {{.*}} monotonic, align 4
+  // CHECK-NEXT: fcmp ult float {{.*}}, {{.*}}
+
+  f = __atomic_max_fetch(&f, 5.0, __ATOMIC_RELAXED);
+  // CHECK:      atomicrmw fmax {{.*}} monotonic, align 4
+  // CHECK-NEXT: fcmp ugt float {{.*}}, {{.*}}
+}

[efriedma-quic]

This is already under review in #160330. (And you can't use fcmp.)

[Lancern]

Closing this as duplication of #160330 .