[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:1304
+
+  // Rejig the  inner condition:
+  // * Use the UpperBound returned from the DynamicNext call.
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > Typo?
> "Rejig" is not a typo? [[ https://en.wiktionary.org/wiki/rejig | Wiktionary  ]] knows it, but UK-only.
Changing to "modify" to be more universal. Living in the UK, I'm not always aware of what English words are "UK" only and which are "any English variety". 


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1734
+      OMPBuilder.createDynamicWorkshareLoop(Loc, CLI, AllocaIP,
+                                            /*NeedsBarrier=*/true, ChunkVal);
+  // The returned value should be the "after" point.
----------------
Meinersbur wrote:
> 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.
I am in the process of adding fortran-compiler tests that do dynamic with no chunk size and static with a given chunk size, to cover more - but won't go into llvm main for a while, I guess.


================
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++));
----------------
Meinersbur wrote:
> [nit] Can use `auto *` when using `dyn_cast` on the same line. At least, please use consistent style in the same function,
The whole file is inconsistent in this respect. First dyn_cast in the file is to a named type (line 156), the second one is to an auto (line 161), but then it is MOSTLY named types, with a scattering of auto in the original static code.  I'm changing the ones below to use non-auto, to match the rest of the file. 


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1780-1782
+  ASSERT_NE(OrigLowerBound, nullptr);
+  ASSERT_NE(OrigUpperBound, nullptr);
+  ASSERT_NE(OrigStride, nullptr);
----------------
Meinersbur wrote:
> 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.
Again, this matches the rest of the file - there is a small number of `cast<X>(y)`, but they are all preceded by an `ASSERT_TRUE(isa<X>(y))`, which is about the same as this construct - the `isa<X>` check may explain more clearly what's wrong. but I doubt anyone working on LLVM for more than a few days will struggle to understand why a `dyn_cast` returned a `nullptr`. I think the assert you get inside the `cast<X>` is less clear - if nothing else, in the sense that you probably can't set a breakpoint and then inspect what happened in a good way. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97393