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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Just a note:

>> 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.

It is not a hack, this was an optimization to minimize the number of recalculations of some expressions for the loops. For master or other directives we just did not need to do it at all.



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76342