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

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1170
+// runtime. Always interpret integers as unsigned similarly to
+// CanonicalLoopInfo.
+static FunctionCallee
----------------
jdoerfert wrote:
> I think we sue doxygen comments `///` for static functions as well.
No problem, I will update - although the other functions in this file aren't using that style (see line 1054-1057) - should I fix all of them (in a separate commit, I expect?) or just the two new functions?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1288
+    It = Inst->eraseFromParent();
+  }
+
----------------
jdoerfert wrote:
> I don't know what this does but it looks brittle. We should have handles for all values, e.g., "the old condition", which we can remove is necessary by following the handle and the operands (with a single use).
Agreed. There needs to be a better way to do this... ;)

What it "does" is: It removes the "instructions after the ones I added" . What I would like to do is actually replace the existing `cond` with a new one[1] (and assuming nothing else uses that original block, remove it). There's no good way to do that with the current interface on CanonicalLoopInfo, so I did this to at least get me to a BasicBlock that doesn't get rejected for having two different terminations (is that the right term?).

[1] That's what I //think// I want to do. My understanding, and this is mainly based on what clang++ generates for "the same" C++ code to the Fortran code I'm experimenting with here, is that my `cond` BB should contain a call to the `kmpc_dispatch_next` and check the return value from that, and either exit or keep going - I may be completely wrong in this understanding. The original code, that I copied from the static variant is doing a compare to the full loop count, but in `dynamic` the work is not necessarily finished in order, happy to be corrected on these things, I'm new to both Fortran and OpenMP,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393