[PATCH] D118547: [OMPIRBuilder] Add support for atomic compare

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1347
 
+  enum AtomicCompareOp : unsigned { EQ, MIN, MAX };
+
----------------
Brief documentation, prefer an enum class, potentially in OpenMPConstants.h


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1372
+  /// \param IsXBinopExpr True if the conditional statement is in the form where
+  ///                     x is on LHS.
+  ///
----------------
Op is missing above.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3530
+          Op == AtomicCompareOp::MAX ? AtomicRMWInst::Max : AtomicRMWInst::Min;
+    }
+    // We dont' need the result for now.
----------------
Once you use a conditional and once a ternary op. Maybe use a helper lambda instead.
Signedness has to come from the user.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118547