ABataev added inline comments.
================
Comment at: lib/AST/DeclOpenMP.cpp:164
+ if (NumClauses) {
+ Clauses = (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
+ setClauses(CL);
----------------
lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > 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?
> > I don't think the Decl requires a lot of memory. Seems to me, it requires
> > ~32 bytes.
> Hi Alexey,
>
> Thanks for the quick response! In the case we discussed earlier, we'll have 2
> entities for a mapper:
>
> ```
> class OMPDeclareMapperDeclContext : public Decl, public DeclContext {...};
>
> class OMPDeclareMapperDecl : public ValueDecl, private TrailingObjects {
> OMPDeclareMapperDeclContext *DC;
> ...
> };
> ```
>
> To me, the `Decl` within `OMPDeclareMapperDeclContext` is useless and
> confusing to people. If you insist to get rid of `OMPClause **Clauses` in the
> current implementation, I propose something below:
>
> We still have 2 entities for a mapper:
>
> ```
> class OMPDeclareMapperClauses : private TrailingObjects {...}
>
> class OMPDeclareMapperDecl : public ValueDecl, public DeclContext {
> OMPDeclareMapperClauses *Clauses;
> ...
> };
> ```
> This seems to be better than the above case. Do you like it?
>
Ok, let's keep the original implementation. But instead of the `OMPClause**`
use `MutableArrayRef<OMPClause*>`
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
