Page MenuHomePhabricator

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

Authored by lildmh on Feb 11 2019, 2:11 PM.

Details

Summary

This patch implements the parsing and sema support for OpenMP map clauses with potential user-defined mapper attached. User defined mapper is a new feature in OpenMP 5.0. A map clause can have an explicit or implicit associated mapper, which instructs the compiler to generate extra data mapping. 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 map(mapper(id) tofrom: ss) // use the mapper with name 'id' to map ss

Diff Detail

Repository
rL LLVM

Event Timeline

lildmh created this revision.Feb 11 2019, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 2:11 PM
lildmh updated this revision to Diff 186338.Feb 11 2019, 2:28 PM

Also, need some kind of the stubbed codegen for the mapper

include/clang/AST/OpenMPClause.h
4236 ↗(On Diff #186338)

The function has more than 10 parameters, it is not too good. Would be good to pack some of them into some kind of structure.

include/clang/Sema/Sema.h
9347 ↗(On Diff #186338)

Also would be good to pack some of the params into structure

9431 ↗(On Diff #186338)

Same, too many params

lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

Why do we need special processing of the mapper here?

1793 ↗(On Diff #186338)

Again, what's so special in mapper lookup?

lib/Sema/SemaOpenMP.cpp
13186–13192 ↗(On Diff #186338)

I think this can be simplified:

if (ER.isInvalid())
  continue;
MVLI.UDMapperList.push_back(ER.get());
13232–13238 ↗(On Diff #186338)

Also can be simplified.

13363–13369 ↗(On Diff #186338)

Again, simplify

lib/Sema/TreeTransform.h
8845 ↗(On Diff #186338)

Enclose into braces

lib/Serialization/ASTReader.cpp
12312 ↗(On Diff #186338)

i->I

lildmh marked 11 inline comments as done.Feb 12 2019, 9:23 AM

Hi Alexey,

Thanks very much for your quick review!

For the codegen, currently I'm thinking to do it within declare target codegen functions. So there is not a very clear interface to do mapper codegen (also, there is not a clear interface to do map clause codegen now), and that's why I didn't have a stub left in this patch. Please see other detailed responses inline.

include/clang/AST/OpenMPClause.h
4236 ↗(On Diff #186338)

I can define a structure OMPMapClauseModifier within OMPMapClause to include all modifier related info. What do you think?

lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

The declare mapper lookup needs to find all OMPDeclareMapperDecls with the same name (the same as declare reduction lookup), and thus it goes through all scopes from the innermost to the outtermost.

Then it looks up a parent scope S (e.g., the outter {} in the following example), all OMPDeclareMapperDecls declared in the children scopes of S will appear in the range between IdResolver.begin(Name) and IdResolver.end(). Also, the declare mapper(id: struct S s) will appear before omp declare mapper(id: struct SS ss). This can cause the lookup function to ignore later omp declare mapper(id: struct SS ss) declared in the outter scope. As a result, we may not find the corrent mapper.

{
  #pragma omp declare mapper(id: struct S s) (s.a)
  {
    #pragma omp declare mapper(id: struct SS ss) (ss.b)
    ...
  }
}

To fix this problem, the purpose of this part of code is to ignore all OMPDeclareMapperDecls in inner scopes, which are already found in previous calls to LookupParsedName() from buildUserDefinedMapperRef.

I also found that the declare reduction lookup has the same problem. I'll push out a similar fix for the declare reduction later.

1793 ↗(On Diff #186338)

The same as above. This is to fix C lookup, and the above is for C++

lib/Serialization/ASTReader.cpp
12312 ↗(On Diff #186338)

I fixed it. Lower case i is also used in other loops in this function. Would you like me to fix them as well?

lildmh updated this revision to Diff 186492.Feb 12 2019, 9:24 AM
lildmh marked 3 inline comments as done.

Fix part of Alexey's comments

Hi Alexey,

Thanks very much for your quick review!

For the codegen, currently I'm thinking to do it within declare target codegen functions. So there is not a very clear interface to do mapper codegen (also, there is not a clear interface to do map clause codegen now), and that's why I didn't have a stub left in this patch. Please see other detailed responses inline.

This till must be handled somehow in codegen, better to ignore for now, I think.

include/clang/AST/OpenMPClause.h
4236 ↗(On Diff #186338)

Yes, it would be good

lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

I don't think this is the correct behavior. For user-defined reductions, the standard says "This match is done by means of a name lookup in the base language." It means we should use the lookup rules of C/C++. For mapper, seems to me, we also should completely rely on the visibility/accessibility rules used by C/C++.

lib/Serialization/ASTReader.cpp
12312 ↗(On Diff #186338)

This is the old code that was committed before the rules were defined. Soon they're going again to update the coding standard to allow lowCamel for the variables, but currently, only CamelCase is used for variables.
No, don't do this.

lildmh marked 2 inline comments as done.Feb 12 2019, 9:56 AM
lildmh added inline comments.
include/clang/AST/OpenMPClause.h
4236 ↗(On Diff #186338)

Then I'll do that. Thanks

lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

Again, thanks for your quick response!

{
  #pragma omp declare mapper(id: struct S s) (s.a)
  {
    #pragma omp declare mapper(id: struct SS ss) (ss.b)
    struct S kk;
    #pragma omp target map(mapper(id), tofrom: kk)
    {}
  }
}

If I don't add this code, the above code will report error that it cannnot find a mapper with name id for type struct S, because it can only find the second mapper for type struct SS. This doesn't look like correct behavior to me.

I think we are still following the lookup rules of C/C++ with this fix. It's because for declare mapper and reduction, we call LookupParsedName multiple times on different scopes. In other cases, LookupParsedName is always called once. Because of this difference, I think this fix is appropriate. What is your thought?

ABataev added inline comments.Feb 12 2019, 10:02 AM
lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

No, in your case we don't. According to the C/C++ rules, if the variable is redeclared, the latest declaration is used. The same rule must be applied to the mapper (according to the standard "The visibility and accessibility of this declaration are the same as those of a variable declared at the same point in the program.")
I think that the code should fail in this case, because you would the error message in case of the variables declarations with the same names in those scopes.

lildmh marked an inline comment as done.Feb 12 2019, 10:19 AM
lildmh added inline comments.
lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

Hi Alexey,

Thanks for your quick response! I don't think it's redeclared in this case, because a mapper has two properties: name and type, just as declare reduction. Only when both name and type are the same, it should be considered as redeclaration.

If we consider the above example as redeclaration of a mapper, then we can always just call LookupParsedName once to find the most relevant mapper/reductor. Then why do you need to search reductors in a while loop in buildDeclareReductionRef?

Also, the following example will be correct using the original lookup functions, without my fix:

{
  #pragma omp declare mapper(id: struct S s) (s.a)
    #pragma omp declare mapper(id: struct SS ss) (ss.b)
    struct S kk;
    #pragma omp target map(mapper(id), tofrom: kk)
    {}
}

If we consider the second mapper as a redeclaration of the first one, this should fail as well. What do you think?

ABataev added inline comments.Feb 12 2019, 10:25 AM
lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

The standard clearly states that "The visibility and accessibility of this declaration are the same as those of a variable declared at the same point in the program.". For variables (btw, they also have 2 attributes - name and type) the inner declaration makes the outer one not visible. The same rule should be applied to the mapper.
Instead of this case, I'd suggest to implement the argument-dependent lookup for the declarations with the same name but with the different types, declared in the same scope.
Your second example should not produce the error as the mappers are declared in the same scope.

lildmh marked an inline comment as done.Feb 12 2019, 10:45 AM
lildmh added inline comments.
lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

Thanks for your patience! I think I got your point about the visibility and accessibility rule. Now I'm confused about the implementation of declare reduction lookup. Could you explain why you use a while loop to call LookupParsedName in buildDeclareReductionRef? I thought I understood your intention when I read your code, but I'm confused now.

For argument dependent lookup, I thought it is for functions. Declare reduction can be considered as a function, but I don't think mappers should be considered as functions. What do you think?

ABataev added inline comments.Feb 12 2019, 10:53 AM
lib/Sema/SemaLookup.cpp
1074 ↗(On Diff #186338)

It just scans through the scopes until it finds at least one UDR with the specified name, nothing else. It does not try to find all the UDRs with the given name in all scopes.
As for the ADL, mapper, in this case, is similar to UDR because you need to find the mapper with the required name and type.

kkwli0 added a subscriber: kkwli0.Feb 12 2019, 11:22 AM
kkwli0 added inline comments.
include/clang/AST/OpenMPClause.h
4077 ↗(On Diff #186492)

assoicated -> associated

4212 ↗(On Diff #186492)

amount -> number?

4229 ↗(On Diff #186492)

assoicated -> associated

lib/Parse/ParseOpenMP.cpp
2144 ↗(On Diff #186492)

Although it is an error situation, will we have different behavior?

... map(xclose, to: x)

Previously, it always parses both modifier and type. After the change, the call of parseMapType is skipped. If it OMPC_MAP_unknown, IsMapTypeImplicit is set.

lildmh marked 4 inline comments as done.
lildmh added inline comments.
lib/Parse/ParseOpenMP.cpp
2144 ↗(On Diff #186492)

Hi Kelvin,

Thanks a lot for the review! It will not change the behavior of parsing modifiers other than mapper. parseMapTypeModifiers will only return true when it finds a invalid mapper for now.

lildmh updated this revision to Diff 186742.Feb 13 2019, 2:20 PM
  1. Move mapper related info to OMPMappableExprListClause, so that to and from clauses can utilize the same infrastructure as well (real support for them will be in another patch);
  2. Combine the function parameters for declare mapper and declare reduction, to reduce the function parameter number of ActOnOMPVarListClause;
  3. Implement ADL for mapper, and remove mapper lookups in SemaLookup.cpp;
  4. Didn't combine modifier related info into a structure, because for to and from clauses will only have possible mapper modifier, not close and always;
  5. Address typos that Kelvin found;
  6. Rebase.

Check, please, that adding this to the map clause does not crash the codegen. Would be good to ignore this construct in codegen for now, if it is used in the code.

include/clang/AST/OpenMPClause.h
4193 ↗(On Diff #186742)

Also would be good to pack the parameters into the structure.

lildmh marked an inline comment as done.Feb 14 2019, 9:32 AM

Hi Alexey,

Again thanks for your review! The codegen completely ignores any mapper related info for now, so it should not crash map clause codegen. It also passed the regression test, so map clause codegen should be fine.

include/clang/AST/OpenMPClause.h
4193 ↗(On Diff #186742)

Among these parameters:

  1. 2 of them are for modifiers. Since mapper can also be used in to and from clauses, it is not good to combine mapper info with them.
  2. 2 of them are mapper info. I can combine MapperQualifierLoc and MapperIdInfo into a structure. It's not a big saving though (only reduce one parameter). Do you want me to do that?
  3. 4 of them are number of vars... It seems not a good idea to combine them into one structure.
  4. The rest are locations. They are kinda all over the place. It doesn't seem a good idea to combine them as well.

Hi Alexey,

Again thanks for your review! The codegen completely ignores any mapper related info for now, so it should not crash map clause codegen. It also passed the regression test, so map clause codegen should be fine.

Hi Lingda, no problems!
It would be good to have at least one codegen test with the mapper construct to prove that it does not crash the codegen. We don't have any codegen test for this new construct.

include/clang/AST/OpenMPClause.h
4193 ↗(On Diff #186742)
  1. The same structure must be used for to and from clauses, not a problem.

2-4. you can combine it with the number of vars and locations into one structure, no?

lildmh marked an inline comment as done.Feb 14 2019, 9:57 AM

Sure I'll add a codegen test with mapper. Thanks!

include/clang/AST/OpenMPClause.h
4193 ↗(On Diff #186742)

None of these locations are for mapper, so I think it's not good to combine them with mapper.
Those numbers (NumVars, NumUniqueDeclarations, NumComponentLists, NumComponents) are not for mapper either. NumVars can be viewed as partially for mapper, but its main purpose is for the number of list items in map clause. So logically, I think they should not be combined with mapper.
What do you think?

ABataev added inline comments.Feb 14 2019, 10:00 AM
include/clang/AST/OpenMPClause.h
4193 ↗(On Diff #186742)

It is not about mapper. We just have too many params in functions and need to reduce it somehow. I don't care about the logic of the parameters packing.

lildmh updated this revision to Diff 186912.Feb 14 2019, 1:46 PM

Introduce a structure OMPMappableExprListSizeTy within OMPMappableExprListClause to aggregate all 4 sizes needed by such clause, and thus reduce the number of parameters when creating a map clause. This can also be used by other similar clauses. I'll have another patch to let other clauses to use this structure as well.

ABataev added inline comments.Feb 15 2019, 6:25 AM
include/clang/AST/OpenMPClause.h
3626 ↗(On Diff #186912)

Add default initializers for the fields

3652 ↗(On Diff #186912)

Remove this constructor, we should use only the one with OMPMappableExprListSizeTy

3682–3685 ↗(On Diff #186912)

I think it is worth to add SourceLocation StartLoc, SourceLocation LParenLoc, SourceLocation EndLoc, to OMPMappableExprListSizeTy. Of course, you need to rename the structure, something like OMPMappableClauseData is good enough. Or you can pack them into a different structure.

4318 ↗(On Diff #186912)

Also too many params, need to "squash" some of them into a structure, maybe several structures.

include/clang/Sema/Sema.h
9431 ↗(On Diff #186912)

Also too many params here.

lildmh updated this revision to Diff 187055.Feb 15 2019, 11:37 AM
lildmh marked 5 inline comments as done.

Further reduce the number of parameters for map, to, from, use_device, and is_device clauses. Also rebase.

include/clang/AST/OpenMPClause.h
3682–3685 ↗(On Diff #186912)

I put StartLoc, LParenLoc, EndLoc into another struct OMPMappableExprListLocTy. I didn't combine them with OMPMappableExprListSizeTy, because some fuctions only use one of them, and some other functions use both of them.

Also modify to, from, is_device, and use_device clauses to use the same interfaces.

ABataev added inline comments.Feb 15 2019, 12:01 PM
include/clang/AST/OpenMPClause.h
3666 ↗(On Diff #187055)

Pass those new structures by const reference, not by value. Do this for all the functions.

lildmh updated this revision to Diff 187070.Feb 15 2019, 12:46 PM

Change parameters to const &

ABataev added inline comments.Feb 15 2019, 1:08 PM
include/clang/Sema/Sema.h
9347 ↗(On Diff #186338)

What about this function?

lildmh updated this revision to Diff 187077.Feb 15 2019, 1:39 PM

Thanks for the catch! I also change the name from OMPMappableExprListLocTy to OMPVarListLocTy to be more accurate. We can potentially factorize other varlist clause code with this. But it will be too large for this patch.

ABataev accepted this revision.Feb 15 2019, 3:12 PM

Thanks for the catch! I also change the name from OMPMappableExprListLocTy to OMPVarListLocTy to be more accurate. We can potentially factorize other varlist clause code with this. But it will be too large for this patch.

Thanks for fixing this. Yes, if you can think we improve something in the code, please, go ahead in the next patches. LGTM!

This revision is now accepted and ready to land.Feb 15 2019, 3:12 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 8:39 AM