[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/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1223-1224
+  // Set up the source location value for OpenMP runtime.
+  if (!updateToLocation(Loc))
+    llvm_unreachable("TODO: Needs to be fixed");
+
----------------
The InsertionPoint part is ignored anyway. It would make more sense to only pass the DebugLoc, like `tileLoops`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1259-1262
+  // TODO: Do we need to calculate the Step if Chunk is not set?
+  //       Currently hard-coded to One!
+  if (!Chunk)
+    Chunk = One;
----------------
For `schedule(dynamic)`, the default chunk size is indeed one.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1266-1267
+
+  // TODO: extract scheduling type and map it to OMP constant. This is curently
+  // happening in kmp.h and its ilk and needs to be moved to OpenMP.td first.
+  // Here we use @kmp_dynamic_chunked | @kmp_sch_modifier_nomonotonic
----------------
The could just be added to `OMPConstants.h`, not necessary to involve tablegen.

`kmp_sched` in `kmp.h` is already redundant with `omp_sched` from omp.h. To not introduce dependency issues atm, I suggest to just reproduce it them `OMPConstants.h` with a comment to remarks they have to keep the in sync.

However, I found that `libomptarget/plugins/amdgpu/src/rtl.cpp` already includes from libLLVMFrontend.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1295
+  // and set IV to the LowerBound.
+  auto *Phi = &*CLI->getHeader()->begin();
+  if (auto *PI = dyn_cast<PHINode>(Phi)) {
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | LLVM's coding style does not use "Almost Always Auto" ]]

Consider using `getHeder()->front()`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1296-1300
+  if (auto *PI = dyn_cast<PHINode>(Phi)) {
+    PI->setIncomingBlock(0, OuterCond);
+    PI->setIncomingValue(0, LowerBound);
+  } else
+    llvm_unreachable("Expected this to be a phi-node");
----------------
`cast` already contains an assertion if the argument is not the casted-to type.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1311-1312
+  // the latch block.
+  // TODO: this can eventually move to CanonicalLoopInfo or to a new
+  // CanonicalLoopInfoUpdater interface.
+  Builder.SetInsertPoint(CLI->getBody(), CLI->getBody()->getFirstInsertionPt());
----------------
Comment is outdated? No CanonicalLoopInfo needs to be preserved.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1348-1350
+  // Don't return CLI - it is no longer a canonical loop.
+  // Just give back the insertion point after the loop.
+  return CLI->getAfterIP();
----------------
Consider extracting all the info you need from CanonicalLoopInfo at the beginning and then abandoning the structure, since starting from the first CFG modification, it does not describe a canonical loop anymore but methods such as `getAfterIP()` may assume so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393