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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:125-130
+  OrderedStaticChunked = 65,
+  OrderedStatic = 66, // ordered static unspecialized
+  OrderedDynamicChunked = 67,
+  OrderedGuidedChunked = 68,
+  OrderedRuntime = 69,
+  OrderedAuto = 70, // ordered auto
----------------
peixin wrote:
> Meinersbur wrote:
> > Did you consider making `ordered` a mask flag like Monotonic/Nonmonotonic?
> These values are copied from "kmp.h". I prefer to keep the originl definition rather than using a mask flag. In addition, the ordered values don't have the consistent mask such as kmp_sch_upper and kmp_ord_upper in "kmp.h".
There are used as flags though: https://github.com/llvm/llvm-project/blob/c94eb0f9ef5597bd1b3b0efda4df53f606a1fe13/openmp/runtime/src/kmp_dispatch.cpp#L185

However, there is an argument to keep the different definitions similar to each other. Eventually I hope those constant can come from a single source.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1892
+                                           /*NeedsBarrier=*/true, ChunkVal,
+                                           /*Ordered*/ true);
   // The returned value should be the "after" point.
----------------
This modifies an existing test to only check with the ordered flag enabled. You could either make an new test for ordered or parametrize this test for with/without ordered.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:690
 
-  if (schedule == omp::ClauseScheduleKind::Static) {
-    ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
-                                         !loop.nowait(), chunk);
-  } else {
+  std::int64_t orderedVal =
+      loop.ordered_val().hasValue() ? loop.ordered_val().getValue() : -1;
----------------
The code would be easier to understand if using a boolean here, e.g. `IsOrdered`.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:767-768
+  } else {
+    ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+                                         !loop.nowait(), chunk);
   }
----------------
Not sure why you inverted the condition statement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114940