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

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 11 inline comments as done.
lildmh added a comment.

Hi Alexey,

Thanks a lot for the review! For 3 places I have doubt. Please see the comments inline. I tried to fix the rest of your comments.



================
Comment at: lib/AST/DeclOpenMP.cpp:164
+  if (NumClauses) {
+    Clauses = (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
+    setClauses(CL);
----------------
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?


================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2868
+TemplateDeclInstantiator::VisitOMPDeclareMapperDecl(OMPDeclareMapperDecl *D) {
+  assert(!D->getType()->isDependentType() &&
+         !D->getType()->isInstantiationDependentType() &&
----------------
ABataev wrote:
> Why?
Thanks! I was wrong about this. Has fixed it in the new patch. Please check


================
Comment at: lib/Serialization/ASTReader.cpp:12304
   for (unsigned i = 0; i != NumVars; ++i)
-    Vars.push_back(Record.readSubExpr());
+    Vars.push_back(Record.readExpr());
   C->setVarRefs(Vars);
----------------
ABataev wrote:
> Restore original
I found using readSubExpr does not work with declare mapper. The reasons are as follows:

readSubExpr will call ASTReader::ReadSubExpr, which will call ReadSubStmt. ReadSubStmt only works with Stmt.

Before, this is correct because map clauses only come with OMPExecutableDirective, which is a Stmt.

Now, map clauses can come with OMPDeclareMapperDecl, which is a Decl. ReadSubStmt does not work with Decl. Instead, readExpr will call ASTReader::ReadExpr. ASTReader::ReadExpr calls ReadSubStmt if it is a Stmt, and it calls ReadStmtFromStream if it is a Decl. The map clause information is indeed in the stream for OMPDeclareMapperDecl. So I use readExpr instead.

This modification should not affect the behavior of map clause serialization for existing directives that are Stmts, since they will both call ReadSubStmt in the end. The regression test confirms that.

Any suggestions?


================
Comment at: lib/Serialization/ASTReader.cpp:12328
   for (unsigned i = 0; i < TotalComponents; ++i) {
-    Expr *AssociatedExpr = Record.readSubExpr();
+    Expr *AssociatedExpr = Record.readExpr();
     auto *AssociatedDecl = Record.readDeclAs<ValueDecl>();
----------------
ABataev wrote:
> Restore original
The same as above.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56326