[PATCH] D76342: [OpenMP] Implement '#pragma omp tile'

[Alexey Bataev via Phabricator via llvm-commits]

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:
> > > > 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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76342/new/

https://reviews.llvm.org/D76342