[PATCH] D99024: [OpenMP][OMPIRBuilder] Adding support for `omp atomic`

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Functionally, I think the patch is fine. There are some formatting issues left.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:921
+  /// \param RMWOp		  The binary operation used for update. If
+  /// operation
+  ///                   is not supported by atomicRMW, or belong to
----------------
[nit] indention


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:932-933
+  ///
+  /// \returns a pair of the old value of X before the update, and the value
+  /// used for the update.
+  std::pair<Value *, Value *> emitAtomicUpdate(Instruction *AllocIP, Value *X,
----------------



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:941-942
+
+  /// emit the binary operation described by \p RMWOp, using \p Src1 and \p Src2
+  /// \Return the Instruction
+  Value *emitRMWOpAsInstruction(Value *Src1, Value *Src2,
----------------
Start sentences with capital letters.

Add empty line between description and \param and \return


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:947
+public:
+  // a struct to pack relevant information while generating atomic Ops
+  struct AtomicOpValue {
----------------
Use a doxygen `///` comment?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2551
+  // 'x' is simply rewritten with some 'expr'.
+  // make monotonic ordering we have different
+  AtomicRMWInst::BinOp AtomicOp = (UpdateExpr ? RMWOp : AtomicRMWInst::Xchg);
----------------
"make monotonic ordering we have different"
incomplete sentence?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2553
+  AtomicRMWInst::BinOp AtomicOp = (UpdateExpr ? RMWOp : AtomicRMWInst::Xchg);
+  auto Result = emitAtomicUpdate(AllocIP, X.Var, Expr, AO, AtomicOp, UpdateOp,
+                                 X.IsVolatile, IsXLHSInRHSPart);
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | The LLVM source style does not use almost always auto ]].


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2082-2088
+  case AtomicRMWInst::Xchg:
+  case AtomicRMWInst::FAdd:
+  case AtomicRMWInst::FSub:
+  case AtomicRMWInst::BAD_BINOP:
+  case AtomicRMWInst::Max:
+  case AtomicRMWInst::Min:
+  default:
----------------
fghanim wrote:
> Meinersbur wrote:
> > fghanim wrote:
> > > Meinersbur wrote:
> > > > With a default case, no need to enumerate all cases.
> > > 
> > > I intentionally listed all cases. If you want, i'll remove `default` which I added to future proof the code.
> > With a default case, the compiler does not warn about unhandled cases anymore. If you want both, you could do:
> > 
> > ```
> >   [...]
> >   case AtomicRMWInst::Min:
> >     llvm_unreachable("Unsupported atomic update operation");
> >   }
> >   llvm_unreacheable("Unhandled atomic update operation");
> > ```
> > Personally I think it just adds clutter lines. In case of some parts of clang's OpenMP implementation it fills entire screens of unsupported cases and there is no chance I miss handling the case during testing due to the llvm_unreachable.
> > 
> > Currently, `return nullptr;` is unreachable code.
> The issue here is that, these cases are either not supported in omp (min,max,..), or should be handled by the cmpExch (fadd, fsub,exch). I worry if i don't explicitly list them as unreachable somebody might try to implement them. 
I like the "OpenMP atomic does not support ..." approach to clarify it is by design.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2295-2306
+  //  for (Instruction &Cur : *EntryBB) {
+  //    if (isa<StoreInst>(Cur)) {
+  //      StoreofAtomic = cast<StoreInst>(&Cur);
+  //      if (StoreofAtomic->getPointerOperand() == XVal)
+  //        continue;
+  //      StoreofAtomic = nullptr;
+  //    }
----------------
Please complete remove commented code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99024