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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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

> Who is going to commit?

I can commit it as soon as you accepted it if you don't mind of course.



================
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:
> ABataev wrote:
> > Meinersbur wrote:
> > > 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.
> > I just prefer not to expose internals if they could be processed within the class instance.
> Using SmallVectorImpl for this is still the dominant style in clang. For instance:
>  * Sema::getUndefinedButUsed
>  * Sema::FindHiddenVirtualMethods
>  * Sema::CollectMultipleMethodsInGlobalPool
>  * Sema::SelectBestMethod
>  * Sema::CollectIvarsToConstructOrDestruct
>  * Sema::collectUnexpandedParameterPacks
>  * Sema::FindProtocolDeclaration
>  * Sema::GatherArgumentsForCall
>  * Sema::GatherGlobalCodeCompletions
>  * Sema::getUndefinedButUsed
>  * ASTContext::getOverriddenMethods
>  * ASTContext::DeepCollectObjCIvars
>  * ASTContext::getInjectedTemplateArgs
> 
>  ...
> 
There is also another motivation behind this solution: to reuse the code as much as possible and avoid code duplication (loops to process inner loops/bodies) and focus just on loops/bodies processing.


================
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:
> ABataev wrote:
> > Meinersbur wrote:
> > > 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) { 
> > > ```
> > > 
> > I believe it just saves us from the extra constructor call.
> It does not: https://godbolt.org/z/81ac7o
Yes, currently it is not, it was before. But even now there is a difference in AST:
  `-CompoundStmt <col:34, line:10:1>
    |-DeclStmt <line:4:5, line:6:6>
    | `-VarDecl <line:4:5, line:6:5> line:4:17 used Lambda '(lambda at line:4:26)':'(lambda at line:4:26)' cinit
    |   `-ExprWithCleanups <col:26, line:6:5> '(lambda at line:4:26)':'(lambda at line:4:26)'
    |     `-CXXConstructExpr <line:4:26, line:6:5> '(lambda at line:4:26)':'(lambda at line:4:26)' 'void ((lambda at line:4:26) &&) noexcept' elidable
    |       `-MaterializeTemporaryExpr <line:4:26, line:6:5> '(lambda at line:4:26)' xvalue



With `&&`:
  `-CompoundStmt <col:34, line:10:1>
    |-DeclStmt <line:4:5, line:6:6>
    | `-VarDecl <line:4:5, line:6:5> line:4:17 used Lambda '(lambda at line:4:26) &&' cinit
    |   `-ExprWithCleanups <col:26, line:6:5> '(lambda at line:4:26)' xvalue
    |     `-MaterializeTemporaryExpr <line:4:26, line:6:5> '(lambda at line:4:26)' xvalue extended by Var 0x562226678ad8 'Lambda' '(lambda at line:4:26) &&'

Just the constructor call is marked as elidable and is not emitted anymore, but it still saves some memory/time because we don't need to emit an extra AST node.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342