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

[Peixin Qiao via Phabricator via llvm-commits]

peixin marked an inline comment as done.
peixin added a comment.

Addressed and replied all @Meinersbur 's questions and concerns.



================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2227
+  CallInst *InitCall = nullptr;
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
----------------
Meinersbur wrote:
> [style] No Almost-Always-Auto
There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2228
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
+    if (isa<CallInst>(cur)) {
----------------
Meinersbur wrote:
> [style] LLVM coding style starts local variables with a capital letter. But preferable, just use `EI.member` instead of `cur->member`. 
There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp. Should I create one NFC patch to fix it in case it guide followers using the wrong style?


================
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;
----------------
Meinersbur wrote:
> 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)
I add some comments. Please check if it is ok to you? You can also fix this to a better one when you support doacross loops.


================
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;
----------------
Meinersbur wrote:
> 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?
There is also one your question I forgot to answer.
> Is the ordered incompatible with static schedules?

For these two questions, I think this is the history issue of clang and openmp runtime library. The `Monotonic` is used by default in openmp runtime library if it is not set here. The ordered is compatible with static schedules. But this code is following what clang does.


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

https://reviews.llvm.org/D114940