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