[PATCH] D102008: [OpenMP]Add support for workshare loop modifier in lowering

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus marked an inline comment as done.
Leporacanthicus added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1436-1442
+    if (SchedType == omp::OMPScheduleType::DynamicChunked ||
+        SchedType == omp::OMPScheduleType::GuidedChunked) {
+      DynamicSchedType |= omp::OMPScheduleType::ModifierNonmonotonic;
+    } else {
+      DynamicSchedType |= omp::OMPScheduleType::ModifierMonotonic;
+    }
+  }
----------------
Meinersbur wrote:
> Leporacanthicus wrote:
> > @Meinersbur & @jdoerfert 
> > 
> > Does this section make sense: use non-monotonic as default for guided and dynamic, and monotonic for Auto and Runtime? From what I can tell, this is what Clang does, and the Fortran compiler complains if I try to use non-monotonic with Auto and Runtime. 
> Try to mimic what the current OpenMP implementation does:
> ```
>   // OpenMP 5.0, 2.9.2 Worksharing-Loop Construct, Desription.
>   // If the static schedule kind is specified or if the ordered clause is
>   // specified, and if the nonmonotonic modifier is not specified, the effect is
>   // as if the monotonic modifier is specified. Otherwise, unless the monotonic
>   // modifier is specified, the effect is as if the nonmonotonic modifier is
>   // specified.
>   if (CGM.getLangOpts().OpenMP >= 50 && Modifier == 0) {
>     if (!(Schedule == OMP_sch_static_chunked || Schedule == OMP_sch_static ||
>           Schedule == OMP_sch_static_balanced_chunked ||
>           Schedule == OMP_ord_static_chunked || Schedule == OMP_ord_static ||
>           Schedule == OMP_dist_sch_static_chunked ||
>           Schedule == OMP_dist_sch_static))
>       Modifier = OMP_sch_modifier_nonmonotonic;
>   }
>   return Schedule | Modifier;
> ```
> 
> i.e. add implicit nonmonotonic only for static schedules and if user did not explicitly specify a modifier. This is `createDynamicWorkshareLoop` so I guess no implicit modifier should be added at all.
Ok, I've removed all the modifier-add-in.

Thanks for your advice!


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1800-1807
+  if (!static_cast<uint32_t>(SchedType & omp::OMPScheduleType::ModifierMask)) {
+    if (SchedType == omp::OMPScheduleType::DynamicChunked ||
+        SchedType == omp::OMPScheduleType::GuidedChunked) {
+      ExpectedSchedType |= omp::OMPScheduleType::ModifierNonmonotonic;
+    } else {
+      ExpectedSchedType |= omp::OMPScheduleType::ModifierMonotonic;
+    }
----------------
Meinersbur wrote:
> Copy&Pasting the code into the tests doesn't seem useful. I'd prefer checking some values and their expected result.
Removed this in the updated patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102008