[Clang] Add support for fp when using min_fetch/max_fetch atomics

[Amichaxx]

Previously when using min_fetch/max_fetch atomics with floating point types, LLVM would emit a crash. This patch updates the EmitPostAtomicMinMax function in CGAtomic.cpp to take floating point types. Included is a clang CodeGen test atomic-ops-float-check-minmax.c and Sema test atomic-ops-fp-minmax.c.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Amina Chabane (Amichaxx)

Changes

Previously when using min_fetch/max_fetch atomics with floating point types, LLVM would emit a crash. This patch updates the EmitPostAtomicMinMax function in CGAtomic.cpp to take floating point types. Included is a clang CodeGen test atomic-ops-float-check-minmax.c.

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGAtomic.cpp (+15-3)

diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index 9106c4cd8e139..99a06e527033b 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -507,20 +507,32 @@ static llvm::Value *EmitPostAtomicMinMax(CGBuilderTy &Builder,
                                          bool IsSigned,
                                          llvm::Value *OldVal,
                                          llvm::Value *RHS) {
+  llvm::Type *ValTy = OldVal->getType();
+
   llvm::CmpInst::Predicate Pred;
+  bool IsFP = ValTy->isFloatingPointTy();
+
   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;
+    Pred = IsFP ? llvm::CmpInst::FCMP_OGT
+                : (IsSigned ? llvm::CmpInst::ICMP_SGT : llvm::CmpInst::ICMP_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;
+    Pred = IsFP ? llvm::CmpInst::FCMP_OLT
+                : (IsSigned ? llvm::CmpInst::ICMP_SLT : llvm::CmpInst::ICMP_ULT);
     break;
   }
-  llvm::Value *Cmp = Builder.CreateICmp(Pred, OldVal, RHS, "tst");
+
+  llvm::Value *Cmp = IsFP
+      ? static_cast<llvm::Value*>(Builder.CreateFCmp(Pred, OldVal, RHS, "tst"))
+      : static_cast<llvm::Value*>(Builder.CreateICmp(Pred, OldVal, RHS, "tst"));
+
   return Builder.CreateSelect(Cmp, OldVal, RHS, "newval");
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[CarolineConcatto]

I saw in this patch:

commit 5cf58768cb3ba31ee37facaf23f7a74f78781590
Author: Tim Northover <[email protected]>
Date:   Thu Nov 21 10:31:30 2019 +0000

that they also added tests Sema tests:
clang/test/Sema/atomic-ops.c
Can we do the same?

I am not sure why it was not added support for floating point at the in that patch, maybe it would be nice to add as well Tim Northover as a reviewer.

[CarolineConcatto]

@Amichaxx I think you need to update your tests. It is failing:
TEST 'Clang :: Sema/atomic-ops.c' FAILED *
You should re-generate the CHECK lines.

[Amichaxx]

@CarolineConcatto Sorry, I've been working on it just forgot to revert.

[Amichaxx]

@CarolineConcatto Thanks for pointing that out, but I am a bit apprehensive to add Tim as he hasn't contributed to the project in nearly 2 years. Otherwise, I have added Sema tests in a separate file.

[Amichaxx]

@efriedma-quic Hi, I've since amended as per your review. Does it look okay? Thanks.

[efriedma-quic]

LGTM with one minor comment.

[CarolineConcatto]

Thank you Amina, can you merge the PR?

[Amichaxx]

@CarolineConcatto I don't have merge access. If you could merge for me, that would be great! Thanks.