[PATCH] D97393: [OpenMP IRBuilder, MLIR] Add support for OpenMP do schedule dynamic

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:109-116
 
+enum class OMPScheduleType {
+  Static = 34, /**< static unspecialized */
+  DynamicChunked = 35,
+  ModifierNonmonotonic =
+      (1 << 30), /**< Set if the nonmonotonic schedule modifier was present */
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierNonmonotonic)
----------------
IMHO we should copy&paste the kmp_schedule enum here entirely, with a comment that it must be kept in sync.

Eventually `kmp.h` should include `OMPConstants.h` and use the declaration here instead of declaring its own.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1304
+
+  // Rejig the  inner condition:
+  // * Use the UpperBound returned from the DynamicNext call.
----------------
Typo?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1236-1237
+
+  if (!Chunk)
+    Chunk = One;
+
----------------
Leporacanthicus wrote:
> Meinersbur wrote:
> > With a Chunk-size that is not one, you need at least two loops: One for all iterations of a chunk and another surrounding while-loop that ask the runtime for the next chunk.
> > 
> > This two-loop structure should also apply to the static schedule if the chunk is set and is greater than one. It don't see this in `createStaticWorkshareLoop`, it might be broken for these cases.
> It appears, from what I can tell, we ALWAYS get one in the chunk-size. I will try to fix this in a bit. I have hand-coded different values in the LLVM-IR to test that the tests I have [written in Fortran] to check that it behaves correctly. Proper tests written in either MLIR or Fortran would be needed for this in the future, and tests that check for example chunk size arrives to this section correctly. 
Consider adding a test to OpenMPIRBuilderTest.cpp that inserts a non-one chunk size, or even a non-constant chunk-size.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393