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