This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP 5.0] Parsing/sema support for to clause with mapper modifier
ClosedPublic

Authored by lildmh on Feb 21 2019, 11:32 AM.

Details

Summary

This patch implements the parsing and sema support for OpenMP to clause with potential user-defined mappers attached. User defined mapper is a new feature in OpenMP 5.0. A to/from clause can have an explicit or implicit associated mapper, which instructs the compiler to generate and use customized mapping functions. An example is shown below:

struct S { int len; int *d; };
#pragma omp declare mapper(id: struct S s) map(s, s.d[0:s.len])
struct S ss;
#pragma omp target update to(mapper(id): ss) // use the mapper with name 'id' to map ss to device

Diff Detail

Repository
rL LLVM

Event Timeline

lildmh created this revision.Feb 21 2019, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 11:32 AM
ABataev added inline comments.Feb 21 2019, 12:00 PM
include/clang/Basic/OpenMPKinds.def
585 ↗(On Diff #187831)

Maybe, better to split it. There must be separate modifiers for from clause and for to clause just for possible future changes.

lib/AST/OpenMPClause.cpp
876 ↗(On Diff #187831)

auto *

924 ↗(On Diff #187831)

auto *

1469 ↗(On Diff #187831)

Better to split this patch into 2: one for to clause and another one for from clause

lib/Parse/ParseOpenMP.cpp
2154 ↗(On Diff #187831)

Remove braces

2174–2175 ↗(On Diff #187831)

mapper(...) does not look good here, we never ever used this kind of construct before. Better to use something that was used already.

lib/Sema/SemaOpenMP.cpp
13061 ↗(On Diff #187831)

Better to fix this in a separate patch.

13356 ↗(On Diff #187831)

Are you sure about this change? This might trigger some asserts.

13413 ↗(On Diff #187831)

Again, better to do this in a separate patch

lildmh marked 12 inline comments as done.Feb 21 2019, 2:21 PM
lildmh added inline comments.
lib/AST/OpenMPClause.cpp
1469 ↗(On Diff #187831)

Hi Alexey,

Thanks for the quick review! The code for to and from clauses are almost identical, so I prefer to have them in the same patch. Besides I don't have commit privilege, so it causes more time to me to have multiple patches. But if you think it's crucial to split it. I can do it. What do you think?

lib/Sema/SemaOpenMP.cpp
13061 ↗(On Diff #187831)

I can do that.

13356 ↗(On Diff #187831)

Yes. The original code before the mapper patch is like this. By moving it out, to and from clauses will also call buildUserDefinedMapperRef check the mapper as well.

13413 ↗(On Diff #187831)

This modification is for to and from clauses, so they also have a default mapper name. I think it is appropriate to have this piece in this patch.

lildmh updated this revision to Diff 187869.Feb 21 2019, 2:52 PM
lildmh marked 3 inline comments as done.
ABataev added inline comments.Feb 22 2019, 6:13 AM
lib/AST/OpenMPClause.cpp
1469 ↗(On Diff #187831)

Hi Lingda, it is clang/LLVM policy to make patches as small as possible (for better "reviewability", reliability, trackability etc.). If it is possible to split the patch into several parts, this must be done.

lildmh updated this revision to Diff 187958.Feb 22 2019, 11:54 AM
lildmh retitled this revision from [OpenMP 5.0] Parsing/sema support for to and from clauses with mapper modifier to [OpenMP 5.0] Parsing/sema support for to clause with mapper modifier.
lildmh edited the summary of this revision. (Show Details)

get rid of the from clause part.

ABataev added inline comments.Feb 22 2019, 12:03 PM
lib/Sema/SemaOpenMP.cpp
13195 ↗(On Diff #187958)

Is this correct for from clause?

13237 ↗(On Diff #187958)

Same here: what about from?

lib/Sema/TreeTransform.h
8821 ↗(On Diff #187958)

llvm::SmallVector<Expr *, 16> & -> llvm::SmallVectorImpl<Expr *> &

8823 ↗(On Diff #187958)

llvm::SmallVector<Expr *, 16> & -> llvm::SmallVectorImpl<Expr *> &

lildmh updated this revision to Diff 187964.Feb 22 2019, 12:13 PM
lildmh marked 4 inline comments as done.
lildmh added inline comments.
lib/Sema/SemaOpenMP.cpp
13195 ↗(On Diff #187958)

Yes, it's correct for from, which will never use MVLI.UDMapperList. Regression test is also correct.

This revision is now accepted and ready to land.Feb 22 2019, 12:23 PM

Thanks a lot!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2019, 2:29 PM