Page MenuHomePhabricator

[OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive
Needs ReviewPublic

Authored by lildmh on Fri, Jan 4, 11:11 AM.

Details

Summary

This patch implements parsing and sema for "omp declare mapper" directive. User defined mapper, i.e., declare mapper directive, is a new feature in OpenMP 5.0. It is introduced to extend existing map clauses for the purpose of simplifying the copy of complex data structures between host and device (i.e., deep copy). An example is shown below:

struct S { int len; int *d; };
#pragma omp declare mapper(struct S s) map(s, s.d[0:s.len]) // Memory region that d points to is also mapped using this mapper.

Diff Detail

Event Timeline

lildmh created this revision.Fri, Jan 4, 11:11 AM
lildmh edited the summary of this revision. (Show Details)Fri, Jan 4, 11:26 AM
lildmh added reviewers: ABataev, hfinkel, Meinersbur.
lildmh added a project: Restricted Project.
ABataev added inline comments.Wed, Jan 9, 6:55 AM
lib/AST/DeclOpenMP.cpp
164

No, bad idea. Use tail allocation for the clauses. Check the implementation of OMPRequiresDecl

lib/CodeGen/CodeGenModule.h
1249

Formatting.

lib/Parse/ParseOpenMP.cpp
503

Enclose substatement in braces.

589

Pass Range by reference rather than as a pointer

lib/Sema/SemaOpenMP.cpp
13590

Restore this comment.

13627

Restore original comment

13923

Restore al the comments that are not related to the patch itself.

lib/Sema/SemaTemplateInstantiateDecl.cpp
2868

Why?

lib/Serialization/ASTReader.cpp
12304

Restore original

12328

Restore original

lildmh marked 11 inline comments as done.Fri, Jan 11, 8:39 AM

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.

lib/AST/DeclOpenMP.cpp
164

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?

lib/Sema/SemaTemplateInstantiateDecl.cpp
2868

Thanks! I was wrong about this. Has fixed it in the new patch. Please check

lib/Serialization/ASTReader.cpp
12304

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?

12328

The same as above.

lildmh updated this revision to Diff 181289.Fri, Jan 11, 8:43 AM
lildmh marked an inline comment as done.
ABataev added inline comments.Fri, Jan 11, 9:15 AM
lib/AST/DeclOpenMP.cpp
164

Instead, you can introduce special DeclContext-based declaration and keep the reference to this declaration inside of the OMPDeclareMapperDecl.

lib/Serialization/ASTReader.cpp
12304

Ok, no problems. I thought it was an accidental change.

lildmh marked an inline comment as done.Fri, Jan 11, 10:08 AM
lildmh added inline comments.
lib/AST/DeclOpenMP.cpp
164

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.

ABataev added inline comments.Fri, Jan 11, 11:16 AM
lib/AST/DeclOpenMP.cpp
164

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.

lildmh marked an inline comment as done.Fri, Jan 11, 11:38 AM
lildmh added inline comments.
lib/AST/DeclOpenMP.cpp
164

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.

ABataev added inline comments.Fri, Jan 11, 11:40 AM
lib/AST/DeclOpenMP.cpp
164

Yes, something like this.