ABataev 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 *> {
----------------
Meinersbur wrote:
> ABataev wrote:
> > Meinersbur wrote:
> > > 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.
> > >
> > No, this is not what I'm asking for. I asked you to think about adding a
> > new level of abstraction somewhere between the `OMPLoop...` and
> > `OMPExecutableDirective` classes to minimize the functional changes and
> > make the classes more robust. Anyway, give me a week or so to try to find
> > possible better abstractions and if there are no better ones, we'll proceed
> > with your original solution.
> > derive `OMPExecutableDirective` and this directive and other similar (+
> > maybe, `OMPSimdDirective`) from this new base class
> sounded like the new class hierarchy should be
> ```
> Stmt -> NewClass -> OMPExecutableDirective -> OMPLoopDirective ->
> OMPForDirective
> \
> -> OMPTileDirective
> \
> -> OMPSimdDirective
> ```
>
> Since `OMPSimdDirective` seems to be affected as well, this seems more like a
> refactoring of the existing structure than something specific to `#pragma omp
> tile`.
>
> Looking forward to your idea.
I have a little bit different scheme in my mind, but you got the main idea. Let
me check, if it works, or I can find a better design.
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
