[PATCH] D90830: [OpenMPIRBuilder] Implement CreateCanonicalLoop.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 5 inline comments as done.
Meinersbur added a comment.

In D90830#2377846 <https://reviews.llvm.org/D90830#2377846>, @fghanim wrote:

> Thanks for the Patch. Generally looks Good.
> Just a couple of very minor comments/questions
>
> - My understanding of the usage case for this, is that when we get `CreateDistribute`, `CreateSimd`, `CreateLoop`, etc., they will use this to generate the loop/s required, but this is not meant to be used directly by clang or flang? is that correct? If yes, why do we have them as public methods, wouldn't it be better to make them private?

OpenMP 5.1 includes the tile and unroll directive that allows chaining loop-associated constructs as controlled by the front-end. I.e.

  #pragma omp for
  #pragma omp unroll partial
  for (int i = 0; i < N; i+=1)

the front-end will do:

  auto LiteralLoop = CreateCanonicalLoop(ForStmt)
  auto UnrolledLoop = CreateUnrollDirective(LiteralLoop );
  CreateWorksharingDirective(UnrolledLoop);

Since LLVMFrontend cannot implement all combinations in which tiling/unrolling/other loop-associated can be combined, the clang/flang must have access to CreateCanonicalLoop to represent the composition in the right order.

The concept is more powerful, e.g. for combined directives:

  auto LiteralLoop = CreateCanonicalLoop(ForStmt)
  auto VectorizedLoop = CreateSimdDirective(LiteralLoop);
  CreateWorksharingDirective(VectorizedLoop);

to implement `#pragma omp for simd` without OMPIRBuilderr necessarily implementing every valid combination of directives.

> - Also, a question; I think when we have something like:
>
>   #pragma omp for
>   for ( ... ; ... ; ...) {
>    //body
>   }
>
> this is going to be encoded in the clang AST as the `omp for` statement, whose body the `ForStatement` node. Now the first will end up using the `CreateForDirective()` that we plan to add in the OMPBuilder, while the latter will basically have clang generate the loop for us ( i.e. using`
> `emitForStatement()` ). is this correct? If yes, wouldn't that mean that there would be some redundancy, or is this for a different case?

Unfortunately `CodeGenFunction::EmitForStmt` does not generate a predictable control-flow, or a way to get the loop's trip count that we need to determine the logical iterations. I.e. we have to implement that ourselves. However, my experimental clang code started its live as a copy&paste of `CodeGenFunction::EmitForStmt` and I try to keep them similar.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:702
+  /// any bit-width.
+  Value *getTripCount() const { return Cond->front().getOperand(1); }
+
----------------
jdoerfert wrote:
> front = getTerminator, right?
> Can we add a few more assertions here and elsewhere, just to make sure the structure is as we expect it.
front is the first instruction in the block, this is the compare instruction for `i < TripCount`. I.e. the second operator is the trip count.

This is already asserted in `assertOK`, but I can add one here as well.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:887
+  CL->assertOK();
+#endif
+  return CL;
----------------
jdoerfert wrote:
> I usually prefer the `assert(CL->isValid() && "...")` style but no objection at the end of the day. (the reason is the other one can be used to dump more of the context easily, e.g., `if (!isValid()) dump_stuff();`.
This follows the pattern `AssertOK` used by LLVM's instructions. I usually prefer this pattern because the crash message includes what is wrong, not just that it is invalid. Dumping stuff also assumes that the data structure is valid.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1403
+  assert(Cond->getSinglePredecessor() == Header);
+  assert(isa<BranchInst>(Header->getTerminator()));
+  assert(size(successors(Cond)) == 2);
----------------
fghanim wrote:
> did you mean to say `Cond->getTerminator()`?
Yep.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90830