[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/include/llvm/Frontend/OpenMP/OMPConstants.h:109-116
 
+enum class OMPScheduleType {
+  Static = 34, /**< static unspecialized */
+  DynamicChunked = 35,
+  ModifierNonmonotonic =
+      (1 << 30), /**< Set if the nonmonotonic schedule modifier was present */
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierNonmonotonic)
----------------
Leporacanthicus wrote:
> Meinersbur wrote:
> > IMHO we should copy&paste the kmp_schedule enum here entirely, with a comment that it must be kept in sync.
> > 
> > Eventually `kmp.h` should include `OMPConstants.h` and use the declaration here instead of declaring its own.
> I have added a comment to say that this needs to match kmp.h
> 
> I did look at copying the whole set of enum values, but I think that's better done at a later stage - I don't feel I understand exactly how these things are being used, and that would be key to how they get organized in a move. [Yes, could just copy the whole thing, but I'm not convinced that is the BEST choice - there appears to be a secondary hierarchy in there].
> 
> I will revisit this in a future patch, after I have spent some time understanding all the uses of these enum values.
Sounds reasonable.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:377
+  /// to update the loop counter.
+  /// \param Loc      The source location description, the insertion location
+  ///                 is not used.
----------------
[suggestion] Add a newline between the description and the beginning of params. Not sure whether it makes a difference for doxygen, but clang-format likes to redo paragraph line wrapping and would include \param into the paragraph.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1289
+  BasicBlock *OuterCond = BasicBlock::Create(
+      PreHeader->getContext(), PreHeader->getName() + ".outer.cond",
+      PreHeader->getParent());
----------------
To avoid creating temporary strings.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1304
+
+  // Rejig the  inner condition:
+  // * Use the UpperBound returned from the DynamicNext call.
----------------
Meinersbur wrote:
> Typo?
"Rejig" is not a typo? [[ https://en.wiktionary.org/wiki/rejig | Wiktionary  ]] knows it, but UK-only.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1734
+      OMPBuilder.createDynamicWorkshareLoop(Loc, CLI, AllocaIP,
+                                            /*NeedsBarrier=*/true, ChunkVal);
+  // The returned value should be the "after" point.
----------------
I assumed you would add a second test when chunk is non-default. However, the default chunk size of 1 is not special, so I am find with this.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1741-1744
+  AllocaInst *PLastIter = dyn_cast<AllocaInst>(&*(AllocaIter++));
+  AllocaInst *PLowerBound = dyn_cast<AllocaInst>(&*(AllocaIter++));
+  AllocaInst *PUpperBound = dyn_cast<AllocaInst>(&*(AllocaIter++));
+  AllocaInst *PStride = dyn_cast<AllocaInst>(&*(AllocaIter++));
----------------
[nit] Can use `auto *` when using `dyn_cast` on the same line. At least, please use consistent style in the same function,


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1780-1782
+  ASSERT_NE(OrigLowerBound, nullptr);
+  ASSERT_NE(OrigUpperBound, nullptr);
+  ASSERT_NE(OrigStride, nullptr);
----------------
You could just use `cast` instead of `dyn_cast` which asserts if it is the wrong type, so you do not need explicitly check for nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393