[PATCH] D101435: [OpenMP][MLIR]Add support for guided, auto and runtime scheduling

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus added a comment.

Will do some minor fixes tomorrow, thanks for the feedback!



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:118
+  Runtime = 37,
+  Auto = 38, /**< auto */
+
----------------
jdoerfert wrote:
> Can we move all of the entries here and use `//< ...` comments maybe.
I'll take a look at moving the others as a separate patch - will fix the comment style in this patch to remind myself that it needs fixing for the others! :)


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1760
   InsertPointTy EndIP =
-      OMPBuilder.createDynamicWorkshareLoop(Loc, CLI, AllocaIP,
+      OMPBuilder.createDynamicWorkshareLoop(Loc, CLI, AllocaIP, GetParam(),
                                             /*NeedsBarrier=*/true, ChunkVal);
----------------
Should use SchedType rather than GetParam()


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:192-194
+      return opInst.emitOpError(
+          "only static (default), dynamic and guided loop "
+          "schedule is currently supported");
----------------
ftynse wrote:
> Haven't we covered all clauses?
Good point. Probably good enough to cover that in the default of the switch at the bottom.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:279-280
+
+    afterIP = ompBuilder->createDynamicWorkshareLoop(
+        ompLoc, loopInfo, allocaIP, schedType, !loop.nowait(), chunk);
   }
----------------
ftynse wrote:
> Could we have a test that at list hits this code path? Does not need to be detailed, just the check that the calls to runtime functions are actually emitted, similarly to what we have for static loops.
The test code that I modified does this - I added the test when adding support for Dynamic scheduling, and this only updated the test to take the scheduling type as a parameter - because that's the only difference from this side - the OpenMP runtime will do different things [I think!]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101435