Meinersbur marked an inline comment as done.
Meinersbur added inline comments.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:4781-4784
+/// This represents the '#pragma omp tile' loop transformation directive.
+class OMPTileDirective final
+ : public OMPLoopDirective,
+ private llvm::TrailingObjects<OMPTileDirective, OMPClause *, Stmt *> {
----------------
ABataev wrote:
> Meinersbur wrote:
> > ABataev wrote:
> > > Not sure that this is a good idea to treat this directive as the
> > > executable directive. To me, it looks like kind of `AttributedStmt`.
> > > Maybe better to introduce some kind of a new base node for this and
> > > similar constructs, which does not own the loop but is its kind of
> > > attribute-like entity?
> > > Also, can we have something like:
> > > ```
> > > #pragma omp simd
> > > #pragma omp tile ...
> > > for(...) ;
> > > ```
> > > Thoughts?
> > While not executed at runtime, syntactically it is parsed like a executable
> > (loop-associated) directive. IMHO it does 'own' the loop, but produces
> > another one for to be owned(/associated) by a different directive, as in
> > your tile/simd example, which should already work. Allowing this was the
> > motivation to do the transformation on the AST-level for now.
> I'm not saying that we should separate parsing of this directive from others,
> it is just better to treat this directive as a little bit different node.
> Currently, it introduces too many changes in the base classes. Better to
> create a new base class, that does not relies on `CapturedStmt` as the base,
> and derive `OMPExecutableDirective` and this directive and other similar (+
> maybe, `OMPSimdDirective`) from this new base class.
Unless you tell me otherwise, `OMPLoopDirective` represents a loop-associated
directive. `#pragma omp tile` is a loop-associated directive.
`OMPLoopDirective` contains all the functionality to parse associated loops,
and unfortunately if derived from `OMPExecutableDirective`.
You seem to ask me to create a new class
"OMPDirectiveAssociatedWithLoopButNotExecutable" that duplicates the parsing
part of "OMPLoopDirective"? This will either be a lot of duplicated code or
result in even more changes to the base classes due to the refactoring.
By the OpenMP specification, simd and tile are executable directives, so
structurally I think the class hierarchy as-is makes sense. From the glossary
of the upcoming OpenMP 5.1:
> An OpenMP directive that appears in an executable context and results in
> implementation code and/or prescribes the manner in which associated user
> code must execute.
Avoiding a CapturedStmt when not needed would a modification of
`clang::getOpenMPCaptureRegions` which currently adds a capture of type
`OMPD_unknown` for such directives. This is unrelated to loop-associated
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
