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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

Who is going to commit?



================
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:
> > 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

 ...



================
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:
> > 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342