[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:2261-2263
+  Builder.restoreIP(EndIP);
+  Builder.CreateRetVoid();
+  OMPBuilder.finalize();
----------------
Finalization (and return) of the function should occur before the EXPECT macros. After all, we want to verify the correctness of the function "when it is ready to use"


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2228
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
+    if (isa<CallInst>(cur)) {
----------------
peixin wrote:
> 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?
Would be nice to do so.

For this patch, I am only looking for the coding style of new code.


================
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;
----------------
peixin wrote:
> 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.
Please mention this in a comment


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

https://reviews.llvm.org/D114940