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: > > > > > > > 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*>` Sure. Will have it done soon. Thanks a lot! 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