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

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus marked 7 inline comments as done.
Leporacanthicus added a comment.

I'm going to upload the fixes once they compile, but it's getting late, so probably not until tomorrow morning.



================
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
----------------
Meinersbur wrote:
> 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.
I have moved a minimal set of constants.


================
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());
----------------
Meinersbur wrote:
> Comment is outdated? No CanonicalLoopInfo needs to be preserved.
Yes, and since we're adding zero to IV, it's not much point in updating it, so I removed the whole block below too - as far as I can tell, it produces the same result.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1236-1237
+
+  if (!Chunk)
+    Chunk = One;
+
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393