[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:1203-1204
+  // Set up the source location value for OpenMP runtime.
+  if (!updateToLocation(Loc))
+    return nullptr;
+
----------------
Leporacanthicus wrote:
> 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.
getOrCreateSrcLocStr uses the InsertionPoint only as backup for the function name. Since it returns a default location when DebugLoc is not available, I don't see in what circumstances it would ever be used. In any case, there should be a version of getOrCreateSrcLocStr that only needs a DebugLoc and possibly a `llvm::Function` which can be obtained from the CanonicalLoopInfo stored BasicBlocks.

I am OK with doing it with a separate change, I was considering myself already.


================
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");
----------------
Leporacanthicus wrote:
> 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"... ;)
This is fine for your personal workflow, but for committing a patch we should aim for clean source code in main. For instance, I often use `auto` locally but to comply to the LLVM coding standard, I have to replace most of them with the actual type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393