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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
