[PATCH] D114940: [OMPIRBuilder] Support ordered clause specified without parameter

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2224
+                                           /*NeedsBarrier=*/true, ChunkVal,
+                                           /*Ordered*/ true);
+
----------------
Prefer consistent style


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2227
+  CallInst *InitCall = nullptr;
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
----------------
[style] No Almost-Always-Auto


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2228
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
+    if (isa<CallInst>(cur)) {
----------------
[style] LLVM coding style starts local variables with a capital letter. But preferable, just use `EI.member` instead of `cur->member`. 


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:251-253
+  let summary = "worksharing-loop construct";
   let description = [{
+    The worksharing-loop construct specifies that the iterations of the loop(s)
----------------
Just confirming this is correct: https://www.openmp.org/spec-html/5.1/openmpsu48.html#x73-730002.11.4


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:802-804
+  // TODO: Handle doacross loop init when orderedVal is greater than 0.
+  int64_t orderedVal =
+      loop.ordered_val().hasValue() ? loop.ordered_val().getValue() : -1;
----------------
Since `ordered` has quite different meaning than `ordered(k)`. I'd prefer to split them into two different variables.

I don't insist on it, but please add a comment on the meanings of orderedVal and the ordered clause, maybe by citing from the the OpenMP spec (for both meanings, not just the "OpenMP 5.1, 2.11.4 " below)


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:872
+    } else {
+      // OpenMP 5.1, 2.11.4 Worksharing-Loop Construct, Desription.
+      // If the static schedule kind is specified or if the ordered clause is
----------------



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:878-879
+      // nonmonotonic modifier is specified.
+      if (!(schedType == llvm::omp::OMPScheduleType::OrderedStatic ||
+            schedType == llvm::omp::OMPScheduleType::OrderedStaticChunked))
+        schedType |= llvm::omp::OMPScheduleType::ModifierNonmonotonic;
----------------
This doesn't seem right. It says "if the ordered clause is specified", but this does not consider the `orderedVal`. What sets the `Monotonic` modifier?


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

https://reviews.llvm.org/D114940