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

[Mats Petersson via Phabricator via llvm-commits]

Leporacanthicus added a comment.

In D97393#2585126 <https://reviews.llvm.org/D97393#2585126>, @Leporacanthicus wrote:

> Please note that this is not intended as the final commit, but rather a basis for asking some advice on how to move forward.
>
> The main stumbling point, which may be my lack of understanding of what it's supposed to do: the CanonicalLoopInfo assumes that the the cond block has a CmpInst as the first instruction. In the dynamic, the corresponding block [in my understanding] starts with a call instruction to fetch the "next" set of data to process. This causes the assertOK to fail, hence it is commented out on line 1300 in the patch.

This is no longer a "not intented as final commit". I'm not saying it's perfect, but review comments would be useful.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1203-1204
+  // Set up the source location value for OpenMP runtime.
+  if (!updateToLocation(Loc))
+    return nullptr;
+
----------------
Meinersbur wrote:
> The InsertionPointTy of `Loc` is not used with methods that apply to a canonical loop. Consider passing only the DebugLoc. The same seem to apply to `createStaticWorkshareLoop`.
It is used (but not really NECESSARY to use!) for to get the debug location in `getOrCreateSrcLocStr` a couple of lines down, which means more changes - I will look at doing this as a separate change, once I have something that works.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1215-1234
+  // Allocate space for computed loop bounds as expected by the "init" function.
+  Builder.restoreIP(AllocaIP);
+  Type *I32Type = Type::getInt32Ty(M.getContext());
+  Value *PLastIter = Builder.CreateAlloca(I32Type, nullptr, "p.lastiter");
+  Value *PLowerBound = Builder.CreateAlloca(IVTy, nullptr, "p.lowerbound");
+  Value *PUpperBound = Builder.CreateAlloca(IVTy, nullptr, "p.upperbound");
+  Value *PStride = Builder.CreateAlloca(IVTy, nullptr, "p.stride");
----------------
Meinersbur wrote:
> Since these are shared with `createStaticWorkshareLoop`, did you consider extractin it into a common function?
Yes, definitely on my mind. I was just concentrating on "get something that works first, then refactor" - otherwise, I find myself refactoring, and then reverting half of that, because it was the "wrong thing"... ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393