Page MenuHomePhabricator

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

Authored by lildmh on Jan 4 2019, 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

Repository
rC Clang

Event Timeline

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

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

lib/CodeGen/CodeGenModule.h
1248

Formatting.

lib/Parse/ParseOpenMP.cpp
502

Enclose substatement in braces.

588

Pass Range by reference rather than as a pointer

lib/Sema/SemaOpenMP.cpp
13719

Restore this comment.

13756

Restore original comment

14052

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

lib/Sema/SemaTemplateInstantiateDecl.cpp
2929

Why?

lib/Serialization/ASTReader.cpp
12303

Restore original

12327

Restore original

lildmh marked 11 inline comments as done.Jan 11 2019, 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
163

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
2929

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

lib/Serialization/ASTReader.cpp
12303

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?

12327

The same as above.

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

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

lib/Serialization/ASTReader.cpp
12303

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

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

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.Jan 11 2019, 11:16 AM
lib/AST/DeclOpenMP.cpp
163

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.Jan 11 2019, 11:38 AM
lildmh added inline comments.
lib/AST/DeclOpenMP.cpp
163

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.Jan 11 2019, 11:40 AM
lib/AST/DeclOpenMP.cpp
163

Yes, something like this.

lildmh added inline comments.Jan 24 2019, 10:43 AM
lib/AST/DeclOpenMP.cpp
163

Hi Alexey,

Sorry for the late response. I was working on something else last week.

When I tried to modify the code based on your suggestions, I found out that DeclContext is only meant to be used for a Decl (please see the comments before class DeclContext {...} in include/clang/AST/DeclBase.h).

It means, if I create a OMPDeclareMapperDeclContext which is a DeclContext but not a Decl, the code cannot work correctly. Therefore OMPDeclareMapperDeclContext must be a Decl itself. If I do it this way, a lot of useless information (all inherited from Decl) will exist within OMPDeclareMapperDeclContext, which is very inefficient.

An alternative way is to have something called OMPDeclareMapperClauses that inherits from TrailingObject to store clause information, and OMPDeclareMapperDecl keeps a pointer that points to OMPDeclareMapperClauses. But I don't think this is better than just having a OMPClause **Clauses, which is my current implementation.

What do you think?

ABataev added inline comments.Jan 24 2019, 1:24 PM
lib/AST/DeclOpenMP.cpp
163

I don't think the Decl requires a lot of memory. Seems to me, it requires ~32 bytes.

lildmh added inline comments.Jan 24 2019, 1:58 PM
lib/AST/DeclOpenMP.cpp
163

Hi Alexey,

Thanks for the quick response! In the case we discussed earlier, we'll have 2 entities for a mapper:

class OMPDeclareMapperDeclContext  : public Decl, public DeclContext {...};

class OMPDeclareMapperDecl : public ValueDecl, private TrailingObjects {
  OMPDeclareMapperDeclContext  *DC;
  ...
};

To me, the Decl within OMPDeclareMapperDeclContext is useless and confusing to people. If you insist to get rid of OMPClause **Clauses in the current implementation, I propose something below:

We still have 2 entities for a mapper:

class OMPDeclareMapperClauses :  private TrailingObjects {...}

class OMPDeclareMapperDecl : public ValueDecl, public DeclContext {
  OMPDeclareMapperClauses *Clauses;
  ...
};

This seems to be better than the above case. Do you like it?

ABataev added inline comments.Jan 25 2019, 6:25 AM
lib/AST/DeclOpenMP.cpp
163

Ok, let's keep the original implementation. But instead of the OMPClause** use MutableArrayRef<OMPClause*>

lildmh added inline comments.Jan 25 2019, 6:42 AM
lib/AST/DeclOpenMP.cpp
163

Sure. Will have it done soon. Thanks a lot!

lildmh updated this revision to Diff 183570.Jan 25 2019, 10:40 AM

Change the type of mapper clause storage from OMPClause ** to MutableArrayRef<OMPClause *>, and rebase

ABataev added inline comments.Jan 25 2019, 12:29 PM
include/clang/AST/DeclBase.h
181

Add comma after 0x2000 to exclude this line from the next pactches

include/clang/AST/DeclOpenMP.h
220

Use /// style of comments in class interface

224

Better to call it MapperVarRef

279

Just Clauses.begin() does not work here?

282

Same here, the conversion looks ugly.

lib/AST/ASTDumper.cpp
383

const auto *

400

const auto *

lib/AST/DeclOpenMP.cpp
136

auto * or just return new ....

144

auto *

150
  1. auto **
  2. Use C.Allocate<OMPClause*>(N)
151

Use makeMutableArrayRef(ClauseStorage, N)

163

Do not use auto here

165

See previous comments for ClauseStorage

lib/AST/DeclPrinter.cpp
1612

Use range-based loop

lib/Parse/ParseOpenMP.cpp
44

Add comma after the new enum

571

Clauses.empty()

lib/Sema/SemaOpenMP.cpp
13492

Text message

13495

Add the text message for the assert

lildmh marked 18 inline comments as done.Jan 25 2019, 1:58 PM

Hi Alexey,

Thanks a lot for the review! Tried to address all of your comments in the new diff.

lildmh updated this revision to Diff 183615.Jan 25 2019, 1:58 PM

Address review comments and rebase

lildmh updated this revision to Diff 184740.Feb 1 2019, 6:55 AM

Rebase.

Hi Alexey,

Thanks a lot for the review! It seems no one else will review it. If you think it's ready to go, could you please accept this patch? I also need help to commit this patch since I don't have the privilege.

ABataev accepted this revision.Feb 1 2019, 7:01 AM

Accepted, just was not marked as accepted for an unknown reason.

This revision is now accepted and ready to land.Feb 1 2019, 7:01 AM
lildmh added a comment.Feb 1 2019, 7:20 AM

Thanks a lot Alexey!

@lildmh Thanks for the patch. I can commit this patch for you. The tests run fine on my machine. However, for committing on other's behalf, we were asked to make the contributor aware that the contribution will be published under a new license. Are you OK with the new license?

@lildmh Thanks for the patch. I can commit this patch for you. The tests run fine on my machine. However, for committing on other's behalf, we were asked to make the contributor aware that the contribution will be published under a new license. Are you OK with the new license?

Thanks a lot Michael! I'm fine with the license.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 12:25 PM
dmajor added a subscriber: dmajor.Aug 2 2019, 11:35 AM

While debugging something else, I noticed that with this patch, Decl now has 33 bits worth of bitfields, so it has gained an extra word. Is that ok? Just want to make sure it wasn't unintentional.

lildmh added a comment.Aug 2 2019, 1:10 PM

Hi David,

An extra bit is needed to be added to IdentifierNamespace, and I could not find another field to shrink its width. As a result, it ends up larger than a word now. If you have a better solution, I'll be happy to know.

Thanks,
Lingda