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

[Alexey Bataev via Phabricator via cfe-commits]

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