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:
> > > > > 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.
> Yes, something like this.
Hi Alexey,
Sorry for the late response. I was working on something else last week.
When I tried to modify the code based on your suggestions, I found out that
`DeclContext` is only meant to be used for a `Decl` (please see the comments
before `class DeclContext {...}` in include/clang/AST/DeclBase.h).
It means, if I create a `OMPDeclareMapperDeclContext ` which is a `DeclContext
` but not a `Decl`, the code cannot work correctly. Therefore
`OMPDeclareMapperDeclContext` must be a `Decl` itself. If I do it this way, a
lot of useless information (all inherited from `Decl`) will exist within
`OMPDeclareMapperDeclContext`, which is very inefficient.
An alternative way is to have something called `OMPDeclareMapperClauses` that
inherits from `TrailingObject` to store clause information, and
`OMPDeclareMapperDecl` keeps a pointer that points to
`OMPDeclareMapperClauses`. But I don't think this is better than just having a
`OMPClause **Clauses`, which is my current implementation.
What do you think?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56326/new/
https://reviews.llvm.org/D56326
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
