[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:
> > 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`.


================
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);
----------------
lildmh wrote:
> 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?
Ok, no problems. I thought it was an accidental change.


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

https://reviews.llvm.org/D56326