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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Sorry for the delay.

Consider starting all sentences in comments with capital letters.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:804-806
+  /// unsigned x = 0;
+  /// #pragma omp atomic update
+  /// x = Expr(x_old); : expr is any legal operation
----------------
Could you insert empty lines, at least separate description from the \param list.

Consider surrounding code with `\code` `\endcode` (https://www.doxygen.nl/manual/commands.html#cmdcode)

`: expr is any legal operation` is not a comment in C/C++. Did you mean to capitalize `expr`?



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1972
+    // to use with but issue the flush call
+    // todo: pass `FlushAO` after memory ordering support is added
+    (void)FlushAO;
----------------
Capitalize "TODO" to make them stand out ([[ https://docs.microsoft.com/en-us/visualstudio/ide/using-the-task-list?view=vs-2019 | some IDEs recognize these ]])


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2005
+    // We need to bitcast and perform atomic op as integer
+    unsigned Addrspace = cast<PointerType>(XTy)->getAddressSpace();
+    IntegerType *IntCastTy =
----------------
There is no test coverage for this non-integer case.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2011
+    LoadInst *XLoad =
+        Builder.CreateLoad(IntCastTy, XBCast, X.IsVolatile, "OMP-atomic-load");
+    XLoad->setAtomic(AO);
----------------
Use a common convention for naming IR registers.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2105
+  llvm_unreachable("Unsupported atomic update operation");
+  // return nullptr;
+}
----------------
Please completely remove commented code.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2152
+    AllocaInst *NewAtomicAddr = Builder.CreateAlloca(XElemTy);
+    NewAtomicAddr->setName(X->getName() + "X.Temp");
+    NewAtomicAddr->moveBefore(AllocIP);
----------------
"X.Temp" could be more descriptive.

Also, they should conform to the lowercase convention 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2166
+        OldExprVal = Builder.CreateBitCast(PHI, XElemTy,
+                                           X->getName() + ".Atomic.FltCast");
+      } else {
----------------
Use lower case for register names


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2006
+
+  OMPBuilder.createAtomicRead(Loc, X, V, AO);
+  LoadInst *AtomicLoad = nullptr;
----------------
Please add
```
EXPECT_FALSE(verifyModule(*M, &errs()));
```
to each tests case to verify its wellformedness.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2010-2011
+
+  for (auto &FI : *EntryBB) {
+    Instruction *Cur = &FI;
+    if (isa<LoadInst>(Cur)) {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Only use auto in selected circumstances. ]]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99024