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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

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.



================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2790-2791
 
+DEF_TRAVERSE_STMT(OMPTileDirective,
+                  { TRY_TO(TraverseOMPExecutableDirective(S)); })
+
----------------
[no change request] OMPLoopBasedDirective is not represented in the visitor.

Well, nor does anything call TraverseOMPLoopDirective, so the possibility to only override the base class traverse-function is unused.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:443
 
+class OMPLoopBasedDirective : public OMPExecutableDirective {
+  friend class ASTStmtReader;
----------------
Please add a documentation what this class represents.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:448
+  /// Number of collapsed loops as specified by 'collapse' clause.
+  unsigned CollapsedNum = 0;
+
----------------
Inaccurate for the tile directive.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:456
+  /// \param EndLoc Ending location of the directive.
+  /// \param CollapsedNum Number of collapsed loops from 'collapse' clause.
+  ///
----------------
Inaccurate for the tile directive.


================
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,
----------------
Please add doxygen comments.

IMHO, using callbacks makes the callers significantly more complicated. Why not fill a SmallVectorImpl<Stmt*> with the result?


================
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);
----------------
I do not see why making this a forwarding reference.


================
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 ||
----------------
Use `isOpenMPLoopDirective()` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342