[PATCH] D127041: [LLVM] Add the support for fmax and fmin in atomicrmw instruction

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1580-1582
+    This indicates which "family" an allocator function is part of. To avoid
+    collisions, the family name should match the mangled name of the primary
+    allocator function, that is "malloc" for malloc/calloc/realloc/free,
----------------
Unrelated whitespace changes


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:44390
   if (N->getOpcode() == ISD::VSELECT && Cond.getOpcode() == ISD::BITCAST &&
-      CondVT.getVectorElementType() == MVT::i1 && Cond.hasOneUse() && 
+      CondVT.getVectorElementType() == MVT::i1 && Cond.hasOneUse() &&
       TLI.isTypeLegal(VT.getScalarType())) {
----------------
Unrelated whitespace change


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:67-69
+    case AtomicRMWInst::FMax:
+    case AtomicRMWInst::FMin:
       return CF->isNaN();
----------------
This is just wrong.

I think this is questionable for the fadd/fsub case since I guess it depends if the payload bits are preserved?


================
Comment at: llvm/lib/Transforms/Utils/LowerAtomic.cpp:78-79
+  case AtomicRMWInst::FMax:
+    NewVal = Builder.CreateFCmpUGT(Loaded, Inc);
+    return Builder.CreateSelect(NewVal, Loaded, Inc, "new");
+  case AtomicRMWInst::FMin:
----------------
This is also wrong. Needs to insert minnum/maxnum intrinsics


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127041/new/

https://reviews.llvm.org/D127041