[PATCH] D76342: [OpenMP] Implement '#pragma omp tile'

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D76342#2535321 <https://reviews.llvm.org/D76342#2535321>, @Meinersbur wrote:

> Thank you for the update.
>
> From comparing the diffs, here is the list of main changes I found:
>
> 1. Create `OMPLoopBasedDirective` as a new base class for OMPLoopDirective (and OMPTileDirective) for CollapsedNum and some utility methods for analyzing the CapturedStmt/ForStmt structure (replacing getTopmostAssociatedStructuredBlock, collectAssociatedLoops)
> 2. Add PreInits child to OMPTileDirective (instead of wrapping them in a CompoundStmt of the transformed body/collectAssociatedLoops)
> 3. Use meaningful SourceLocations instead of my placeholder `{}`
> 4. New OMPTransformDirectiveScopeRAII
>   1. to collect nested PreInits (instead by collectAssociatedLoops)
>   2. to assign values to CapturedDecls (instead of adding a `Capturing` argument to various function)
> 5. no call to checkOpenMPLoop for the entire lop nest (but still once per loop)
>
> My remarks to these changes:
>
> 1. Saves us to store all the loop-specific subexpressions in the AST node. However, they are still computed in ActOnOpenMPTileDirective. With the OpenMPIRBuilder (D94973 <https://reviews.llvm.org/D94973>) these will also not be necessary for other loop-associated statements.
> 2. looks more consistent with other directives
> 3. This was just laziness on my size. Thank you.
> 4. is what I just did not know. 4.b. still feels like a hack because there are captures variables outside of any CapturedStmt and therefore complicated the AST. Comparable directive such as `master` don't do this.
> 5. The additional call on the entire loop nest felt necessary to check constraints such as invariant loop bounds and disallow nesting. However, both properties are still checked now.

Just one thing. Preinits does not directly relate to CapturedStmt, you can consider it as a temp way to optimize the codegen. Also, it allows to support the use of data members as loop counters in classes/structures.



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:448
+  /// Number of collapsed loops as specified by 'collapse' clause.
+  unsigned CollapsedNum = 0;
+
----------------
Meinersbur wrote:
> Inaccurate for the tile directive.
We can rename it to something like `NumAssociatedLoops`


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:651-677
+  static bool
+  doForAllLoops(Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+                unsigned NumLoops,
+                llvm::function_ref<bool(unsigned, Stmt *)> Callback);
+  static bool
+  doForAllLoops(const Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+                unsigned NumLoops,
----------------
Meinersbur wrote:
> Please add doxygen comments.
> 
> IMHO, using callbacks makes the callers significantly more complicated. Why not fill a SmallVectorImpl<Stmt*> with the result?
It just hides all internal representation in the class and the user/caller should not worry about proper implementation of the loops processing. Consider it as a way to encapsulate representation details.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:659
+                llvm::function_ref<bool(unsigned, const Stmt *)> Callback) {
+    auto &&NewCallback = [Callback](unsigned Cnt, Stmt *CurStmt) {
+      return Callback(Cnt, CurStmt);
----------------
Meinersbur wrote:
> I do not see why making this a forwarding reference.
There is a type mismatch in callback types, `Stmt *` and `const Stmt *`


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:680-709
+    return T->getStmtClass() == OMPSimdDirectiveClass ||
+           T->getStmtClass() == OMPForDirectiveClass ||
+           T->getStmtClass() == OMPTileDirectiveClass ||
+           T->getStmtClass() == OMPForSimdDirectiveClass ||
+           T->getStmtClass() == OMPParallelForDirectiveClass ||
+           T->getStmtClass() == OMPParallelForSimdDirectiveClass ||
+           T->getStmtClass() == OMPTaskLoopDirectiveClass ||
----------------
Meinersbur wrote:
> Use `isOpenMPLoopDirective()` instead?
I'll check if it can be used instead, though I just want to be consistent with what we have for other statements/expressions/directives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342