Meinersbur added a comment.
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.
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2790-2791
+DEF_TRAVERSE_STMT(OMPTileDirective,
+ { TRY_TO(TraverseOMPExecutableDirective(S)); })
+
----------------
[no change request] OMPLoopBasedDirective is not represented in the visitor.
Well, nor does anything call TraverseOMPLoopDirective, so the possibility to
only override the base class traverse-function is unused.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:443
+class OMPLoopBasedDirective : public OMPExecutableDirective {
+ friend class ASTStmtReader;
----------------
Please add a documentation what this class represents.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:448
+ /// Number of collapsed loops as specified by 'collapse' clause.
+ unsigned CollapsedNum = 0;
+
----------------
Inaccurate for the tile directive.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:456
+ /// \param EndLoc Ending location of the directive.
+ /// \param CollapsedNum Number of collapsed loops from 'collapse' clause.
+ ///
----------------
Inaccurate for the tile directive.
================
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,
----------------
Please add doxygen comments.
IMHO, using callbacks makes the callers significantly more complicated. Why not
fill a SmallVectorImpl<Stmt*> with the result?
================
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);
----------------
I do not see why making this a forwarding reference.
================
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 ||
----------------
Use `isOpenMPLoopDirective()` instead?
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
