ABataev added a comment.
In D76342#2535321 <https://reviews.llvm.org/D76342#2535321>, @Meinersbur wrote:
> 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.
Just one thing. Preinits does not directly relate to CapturedStmt, you can
consider it as a temp way to optimize the codegen. Also, it allows to support
the use of data members as loop counters in classes/structures.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:448
+ /// Number of collapsed loops as specified by 'collapse' clause.
+ unsigned CollapsedNum = 0;
+
----------------
Meinersbur wrote:
> Inaccurate for the tile directive.
We can rename it to something like `NumAssociatedLoops`
================
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:
> 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.
================
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:
> I do not see why making this a forwarding reference.
There is a type mismatch in callback types, `Stmt *` and `const Stmt *`
================
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 ||
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76342/new/
https://reviews.llvm.org/D76342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
