[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Lingda Li via Phabricator via cfe-commits]

lildmh marked an inline comment as done.
lildmh added inline comments.


================
Comment at: lib/AST/DeclOpenMP.cpp:164
+  if (NumClauses) {
+    Clauses = (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
+    setClauses(CL);
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > No, bad idea. Use tail allocation for the clauses. Check the implementation of `OMPRequiresDecl`
> > > > I think it is possible to use TrailingObjects for clause storage when the number of clauses are known before creating the directive (e.g., for OMPRequiresDecl and OMPExecutableDirective). 
> > > > 
> > > > The reason that I had to create OMPDeclareMapperDecl before parsing map clauses, is the mapper variable (AA in the example below) needs to be declared within OMPDeclareMapperDecl, because the following map clauses will use it.
> > > > 
> > > > ```
> > > > #pragma omp declare mapper(struct S AA) map(AA.field1)
> > > > ```
> > > > 
> > > > A possible way to get around this is to count the number of map clauses before hand. But this solution is not trivial since the normal method for parsing map clauses cannot be used (e.g., it does not know AA when parsing map(AA.field1)). A customized and complex (because it needs to handle all possible situations) parsing method needs to be created, just for counting clause number. I think it's not worthy to do this compared with allocating map clause space later.
> > > > 
> > > > I checked the code for OMPDeclareReductionDecl that you wrote. It also has to be created before parsing the combiner and initializer. It does not have a variable number of clauses though.
> > > > 
> > > > Any suggestions?
> > > Instead, you can introduce special DeclContext-based declaration and keep the reference to this declaration inside of the `OMPDeclareMapperDecl`.
> > Hi Alexey,
> > 
> > Thanks a lot for your quick response! I don't think I understand your idea. Can you establish more on that?
> > 
> > In my current implementation, OMPDeclareMapperDecl is used as the DeclConext of the variable AA in the above example, and it already includes the reference to AA's declaration.
> > 
> > My problem is, I need to create OMPDeclareMapperDecl before parsing map clauses. But before parsing map clauses, I don't know the number of clauses. Using TrailingObject requires to know how many clauses there are when creating OMPDeclareMapperDecl. So I couldn't use TrailingObject.
> > 
> > My current solution is to create OMPDeclareMapperDecl before parsing map clauses, and to create the clause storage after parsing finishes.
> What I meant, that you don't need to use `OMPDeclareMapperDecl` for this, instead you can add another (very simple) special declaration based on `DeclContext` to use it as the parent declaration for the variable. In the `OMPDeclareMapperDecl` you can keep the reference to this special declaration.
Thanks for your response! Please let me know if my understanding below is correct:

`OMPDeclareMapperDecl` no longer inherits from `DeclContext`. Instead, we create something like `OMPDeclareMapperDeclContext` which inherits from `DeclContext`, and `OMPDeclareMapperDecl` keeps a pointer that points to this `OMPDeclareMapperDeclContext`.  AA and map clauses are parsed within `OMPDeclareMapperDeclContext`.

This sounds a bit more complex, but if you believe it's better, I can change the code. Please share your thoughts.


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

https://reviews.llvm.org/D56326