[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 comments.



================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2228
+  for (auto &EI : *Preheader) {
+    Instruction *cur = &EI;
+    if (isa<CallInst>(cur)) {
----------------
Meinersbur wrote:
> 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.
OK. Will do it after this patch.


================
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:
> 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
Added in comments.


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

https://reviews.llvm.org/D114940