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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.


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


================
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,
----------------
ABataev wrote:
> 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.
That could also be done by returning and iterator/filling an array with the requested data. The latter is one of the primary uses of `SmallVectorImpl`.

coroutines should make returning an iterator much simpler, eventually when we are allowed to use C++20.


================
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);
----------------
ABataev wrote:
> Meinersbur wrote:
> > I do not see why making this a forwarding reference.
> There is a type mismatch in callback types, `Stmt *` and `const Stmt *`
I understand why the additional lambda is needed, but wondered about why the declaration is not
```
auto NewCallback = [Callback](unsigned Cnt, Stmt *CurStmt) { 
```



================
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 ||
----------------
ABataev wrote:
> 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.
It would reduce the maintenance cost of not forgetting to update this list as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342