[PATCH] D116292: [OMPIRBuilder][MLIR] Support ordered clause specified with parameter

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thank you for the summary, it was helpful.

> With this change, lowering to LLVM IR is supported when dynamic schedule

is specified and collapse value is greater than 1. Also add the test
case.

Could you explain what goes bad when you do not do this?

In D116292#3210335 <https://reviews.llvm.org/D116292#3210335>, @peixin wrote:

> Clang transforms the doacross loop nest into a new one with lower bound of 0 and step of 1. However, this is really not necessary. OpenMP runtime can handle the doacross loop nest regardless of positive or negative step https://github.com/llvm/llvm-project/blob/7c3cf4c2c0689be1a08b8a1326703ec5770de471/openmp/runtime/src/kmp_csupport.cpp#L4050-L4058. The doacross loop nest is independent of worksharing-loop.

While it does contain code for it, it is also wrong in edge case:

1. If lo is larger than up (I assume there must be check for this somewhere, but I don't who is responsible for checking it; the compiler-emitted code?)
2. If `lo - up` overflows, in particular if the loop counter variable itself is `int64_t`. The trip count itself doesn't even need to be large if the increment value is large as well.
3. Potentially if the loop counter variable is `uint64_t` and lo/up larger than 2^63.

An integer loop variable must be introduced anyway for loops over iterators, we might just as well normalize everything to a simplified logical iteration space and not have to bother with overflows later.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:424
+  ///                     loops.
+  void applyDoacrossLoop(DebugLoc DL, InsertPointTy AllocaIP,
+                         BasicBlock *PreHeaderBB, BasicBlock *ExitBB,
----------------
Did you consider making doacross part of an existing call like applyDynamicWorkshareLoop? What are the reason against it? If is a potential `collapseLoop` that loses information of the dimensionality of the original loop, did you consider adding that information to `CanonicalLoopInfo` such that it can be preserved?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1718-1720
+  for (size_t I = 0; I < DoacrossVars.size(); I++)
+    assert(DoacrossVars[I]->getType()->isIntegerTy(64) &&
+           "Doacross init runtime call requires loop bounds with i64 type");
----------------
If the body of the loop is just an assert, enclose the entire loop into an `#ifndef NDEBUG`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1729-1732
+  SmallVector<Type *, 3> ElementsTys;
+  ElementsTys.emplace_back(Int64); // lower
+  ElementsTys.emplace_back(Int64); // upper
+  ElementsTys.emplace_back(Int64); // stride(step)
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1744
+  // Store doacross loop vars in loop bounds.
+  for (std::int64_t I = 0; I < OrderedVal; I++) {
+    Value *LoopBounds = Builder.CreateInBoundsGEP(
----------------



================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2025-2115
+  auto AllocaIter = BB->begin();
+  ASSERT_GE(std::distance(BB->begin(), BB->end()), 5);
+  AllocaIter++; // PLastIter
+  AllocaIter++; // PLowerBound
+  AllocaIter++; // PUpperBound
+  AllocaIter++; // PStride
+  AllocaInst *DIMS = dyn_cast<AllocaInst>(&*(AllocaIter));
----------------
I don't think this kind of checking is useful. It does not make clear what properties are actually relevant and very difficult to update even if e.g. just the allocas are ordered differently.

I suggest to only have some sanity checks, such as the existence of a call to `__kmpc_doacross_fini`. 


================
Comment at: mlir/test/Dialect/OpenMP/invalid.mlir:123
   // expected-error at +1 {{ordered is not a valid clause for the omp.parallel operation}}
-  omp.parallel ordered(2) {}
+  omp.parallel ordered(0) {}
 }
----------------
Why this change?


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

https://reviews.llvm.org/D116292