This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)
ClosedPublic

Authored by jdenny on Jul 2 2020, 10:20 AM.

Details

Summary

This patch implements Clang front end support for the OpenMP TR8
present map type modifier. The next patch in this series implements
OpenMP runtime support.

This patch does not implement the present implicit-behavior for
defaultmap or the present motion-modifier for target update.
These can be implemented in subsequent patches.

A present map type modifier behavior that this patch does not
attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319,
L14-16:

If a map clause with a present map-type-modifier is present in a map
clause, then the effect of the clause is ordered before all other
map clauses that do not have the present modifier.

Compare to L10-11:

For a given construct, the effect of a map clause with the to, from,
or tofrom map-type is ordered before the effect of a map clause with
the alloc, release, or delete map-type.

I have some questions before this patch is ready for a detailed
review:

  1. Is either passage quoted above relevant for any case that does not involve aliasing?
  1. As far as I can tell, Clang does not implement L10-11 to handle aliasing. Is it expected that it already does? If not, then I think both passages should be implemented together later. Any objections?
  1. What should the order be in the case of map(tofrom:expr1) map(present,alloc:expr2)?
  1. The Clang codegen test files are difficult to maintain because of their sizes, but I tried to insert present tests into them anyway to be consistent with the existing always and close tests. I'd like to move all present codegen tests to separate test files. Any objections?

Diff Detail

Event Timeline

jdenny created this revision.Jul 2 2020, 10:20 AM
jdenny updated this revision to Diff 277215.Jul 10 2020, 8:36 PM

Rebased.

Ping. Maybe we can start with the simplest question from the review summary:

The Clang codegen test files are difficult to maintain because of their sizes, but I tried to insert present tests into them anyway to be consistent with the existing always and close tests. I'd like to move all present codegen tests to separate test files. Any objections?

jdenny added inline comments.Jul 15 2020, 8:35 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

In today's OpenMP/LLVM call, it was pointed out that this issue represents a general Clang bug for map clauses: map clauses generally shouldn't be ignored just because their variables aren't referenced in the construct.

Here, I tried to handle this issue only in the case of a present modifier because I previously didn't consider whether the existing behavior for other map types was a bug. My solution here was similar to what's done for omp target data, which doesn't have captures at all. Perhaps, this should generalized to other map types.

Another possibility might be to expand what's captured in Sema.

ABataev added inline comments.Jul 15 2020, 9:00 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

It must be fixed in a separate patch. We definitely should not modify sema here, just the codegen. Currently, it iterates through all captures in region, also need to iterate through the explicit maps.

jdenny added inline comments.Jul 15 2020, 9:08 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

Thanks for the response. I'd be happy to generalize and extract into a new patch.

It looks like all I need to extract is the changes to generateAllInfo and emitTargetCall except I wouldn't have the PresentModifierOnly restriction. Of course, I would need to add tests. Does all that sound about right? If so, I'll work on it.

ABataev added inline comments.Jul 15 2020, 9:28 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

Yes, something like this. Most probably, you won't need to add new tests, just modify the existing ones.

jdenny added inline comments.Jul 15 2020, 1:25 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

OK, I'm working on this. One point from the call I'm not confident about: Is this change just for globals or locals too? That is, which variables in the following examples should have map entries?

int x;
void fn() {
  int y;
  #pragma omp target map(x,y)
  {
    // no references to x and y are lexically contained
  }
}
ABataev added inline comments.Jul 15 2020, 1:30 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
9582

I think, both. We need to have references in the data environment for all explicitly mapped variables.

jdenny updated this revision to Diff 278343.Jul 15 2020, 6:08 PM
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rG LLVM Github Monorepo.

Rebased, and extracted D83922 as discussed.

jdenny marked 6 inline comments as done.Jul 15 2020, 6:11 PM

This new modifier must be enabed only for OpenMP 5.1, need to add the checks for OpenMP version. Also, this change should not affect the functionality of previous versions

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7046–7047

Better to use thу next value to avoid compatibility issues with XLC.

jdenny marked an inline comment as done.Jul 16 2020, 4:11 PM
jdenny added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7046–7047

You mean 0x1000?

jdenny updated this revision to Diff 278658.Jul 16 2020, 9:11 PM

Rebased onto latest D83922.

Implemented rejection of present if not OpenMP >= 5.1.

Cleaned up tests some.

ABataev added inline comments.Jul 17 2020, 7:44 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1254–1255

Better to keep original message for <= 5.0. This is what we usually do

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7046–7047

Yes

jdenny added inline comments.Jul 17 2020, 8:12 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1254–1255

What message do you mean? Which err_ diag id?

ABataev added inline comments.Jul 17 2020, 8:17 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1254–1255

For <= 5.0 the error message should be the same.

def err_omp_unknown_map_type_modifier : Error<
  "incorrect map type modifier, expected 'always', 'close', %select{or 'mapper'|'mapper', or 'presence'}0">;

In code:

Diag(..., diags::err_omp_unknown_map_type_modifier) << (LangOpts.OpenMP <= 50 ? 0 : 1);
jdenny updated this revision to Diff 278877.Jul 17 2020, 12:31 PM

Adjusted diagnostics as requested.

Adjusted OMP_MAP_PRESENT as requested.

jdenny marked 5 inline comments as done.Jul 17 2020, 12:32 PM
ABataev added inline comments.Jul 17 2020, 2:04 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7046–7047

Could you add a comment that 0x800 is currently reserved for compatibility, please?

7979–7985

Why do we need this extra stuff here?

clang/lib/Parse/ParseOpenMP.cpp
3079–3080

Why do we need this?

3142

Better to use (getLangOpts().OpenMP >= 51 ? 1 : 0), the select is of integer type.

3163–3164

Same, why do we need this?

jdenny marked 4 inline comments as done.Jul 17 2020, 3:22 PM
jdenny added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

When called with OMPC_map, getOpenMPSimpleClauseType can return either an OpenMPMapModifierKind or OpenMPMapClauseKind depending on Tok. Thus, without this change, both isMapModifier and isMapType can return either of those despite the difference in their names and documentation.

I found that, when Parser::parseMapTypeModifiers ignores present as if it's not a modifier because OpenMP < 5.1, then isMapType later returns OMPC_MAP_MODIFIER_present and causes the following assert to fail in Sema::ActOnOpenMPVarListClause:

assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
           "Unexpected map modifier.");

To me, the most obvious solution is to fix isMapType and isMapModifier. Fortunately, looking in OpenMPKinds.h, the enumerator values in OpenMPMapModifierKind and OpenMPMapClauseKind are disjoint so we can tell which we have.

3163–3164

Responded on isMapModifier.

jdenny marked 2 inline comments as done.Jul 17 2020, 3:23 PM
jdenny marked 2 inline comments as done.Jul 17 2020, 4:38 PM
jdenny added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7979–7985

For example:

#pragma omp target map(present, tofrom: s.x[0:3])

This generates 2 map entries:

  • s: 0x1020
  • s.x: 0x1000000001003

Without the above change, the 0x1000 is missing from the first entry. As a result, s is allocated, and then the presence check for s.x incorrectly passes at run time.

jdenny marked an inline comment as done.Jul 17 2020, 4:38 PM
jdenny updated this revision to Diff 278934.Jul 17 2020, 4:53 PM

Rebased onto a more recent master.

Added requested comment about skipping 0x800.

As requested: getLangOpts().OpenMP >= 51 -> getLangOpts().OpenMP >= 51 ? 1 : 0.

jdenny marked 2 inline comments as done.Jul 17 2020, 4:54 PM
ABataev added inline comments.Jul 20 2020, 10:00 AM
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

Can we have something like:

if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
  return OMPC_MAP_MODIFIER_unknown;

or extend getOpenMPSimpleClauseType function with the version parameter and check there is modifier is allowed and return unknown if it is not compatible with provided version?

3112–3113

Update a comment here

clang/test/OpenMP/target_data_codegen.cpp
258–260

Add a comment with interpretaion of the map flags, like OMP_TO = 0x1 | OMP_FROM=0x2 | OMP_PRESENT = 0x1000 = 0x1003

jdenny marked 2 inline comments as done.Jul 20 2020, 2:24 PM
jdenny added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

As far as I can tell, my changes general fix this bug in isMapType and isMapModifier. It makes them behave as documented. The suggestions you're making only fix them for the case of present. Why is that better?

clang/test/OpenMP/target_data_codegen.cpp
258–260

Are you asking me to add that comment to every map type encoding appearing in this patch? Or just this one?

ABataev added inline comments.Jul 21 2020, 8:42 AM
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

It is too general. I think it may mask some issues in future. That's why I would suggest to tweak it for present only. Or, even better, extend getOpenMPSimpleClauseType function to handle this modifiers more correctly for each particular version rather than fix it here.

clang/test/OpenMP/target_data_codegen.cpp
258–260

It would be good to have something like this in all codegen tests you added in this patch

jdenny marked an inline comment as done.Jul 21 2020, 10:41 AM
jdenny added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

Or, even better, extend getOpenMPSimpleClauseType function to handle this modifiers more correctly for each particular version rather than fix it here.

One way to implement what I think you mean is to add an additional parameter to getOpenMPSimpleClauseType to request either the clause's associated type or that type's modifiers. Unless I missed a clause, this parameter would affect only map, defaultmap, and schedule clauses. For other clauses, this parameter would be ignored. Is that what you have in mind?

jdenny updated this revision to Diff 279578.Jul 21 2020, 10:51 AM

Adjusted comments as requested.

jdenny marked 3 inline comments as done.Jul 21 2020, 10:53 AM
ABataev added inline comments.Jul 21 2020, 11:11 AM
clang/lib/Parse/ParseOpenMP.cpp
3079–3080

I would also add a check for version compatibility if the modifier is supported for the given version. But anyway, I would like to take a look at what you have in mind

jdenny updated this revision to Diff 279608.Jul 21 2020, 12:28 PM
jdenny marked 4 inline comments as done.

Move present filter for OpenMP < 5.1 from parseMapTypeModifiers to getOpenMPSimpleClauseType.

Remove isMapType and isMapModifier fixes, which are no longer needed for this patch. However, add fixmes explaining how they don't always behave as documented.

clang/lib/Parse/ParseOpenMP.cpp
3079–3080

I think I misread your previous comment. I believe I've implemented your suggestion now.

ABataev accepted this revision.Jul 21 2020, 12:31 PM

LG. Please, land it after the runtime part

This revision is now accepted and ready to land.Jul 21 2020, 12:31 PM
jdenny added a comment.EditedJul 21 2020, 12:43 PM

LG.

Thanks for the review.

As discussed in the review summary, please consider the following. A present map type modifier behavior that this patch does not attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:

If a map clause with a present map-type-modifier is present in a map
clause, then the effect of the clause is ordered before all other
map clauses that do not have the present modifier.

Compare to L10-11:

For a given construct, the effect of a map clause with the to, from,
or tofrom map-type is ordered before the effect of a map clause with
the alloc, release, or delete map-type.

As far as I can tell, Clang does not implement L10-11. Is that correct? If not, then I think both passages should be implemented together later. Any objections?

Please, land it after the runtime part

I'll push them at the same time. The runtime patch needs to be second because its test suite depends on this patch. But there are still some details to resolve in the runtime patch as well.

LG.

Thanks for the review.

As discussed in the review summary, please consider the following. A present map type modifier behavior that this patch does not attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:

If a map clause with a present map-type-modifier is present in a map
clause, then the effect of the clause is ordered before all other
map clauses that do not have the present modifier.

Compare to L10-11:

For a given construct, the effect of a map clause with the to, from,
or tofrom map-type is ordered before the effect of a map clause with
the alloc, release, or delete map-type.

As far as I can tell, Clang does not implement L10-11. Is that correct? If not, then I think both passages should be implemented together later. Any objections?

Looks like you're right. Yes, go ahead and implement it.

Please, land it after the runtime part

I'll push them at the same time. The runtime patch needs to be second because its test suite depends on this patch. But there are still some details to resolve in the runtime patch as well.

LG.

Thanks for the review.

As discussed in the review summary, please consider the following. A present map type modifier behavior that this patch does not attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:

If a map clause with a present map-type-modifier is present in a map
clause, then the effect of the clause is ordered before all other
map clauses that do not have the present modifier.

Compare to L10-11:

For a given construct, the effect of a map clause with the to, from,
or tofrom map-type is ordered before the effect of a map clause with
the alloc, release, or delete map-type.

As far as I can tell, Clang does not implement L10-11. Is that correct? If not, then I think both passages should be implemented together later. Any objections?

Looks like you're right. Yes, go ahead and implement it.

Are you ok for it to be a later patch after pushing these?

LG.

Thanks for the review.

As discussed in the review summary, please consider the following. A present map type modifier behavior that this patch does not attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:

If a map clause with a present map-type-modifier is present in a map
clause, then the effect of the clause is ordered before all other
map clauses that do not have the present modifier.

Compare to L10-11:

For a given construct, the effect of a map clause with the to, from,
or tofrom map-type is ordered before the effect of a map clause with
the alloc, release, or delete map-type.

As far as I can tell, Clang does not implement L10-11. Is that correct? If not, then I think both passages should be implemented together later. Any objections?

Looks like you're right. Yes, go ahead and implement it.

Are you ok for it to be a later patch after pushing these?

Sure

Thanks. It probably won't be soon. At the very least, I need to find the answer to question 3 from the review summary.

jdenny updated this revision to Diff 279681.Jul 21 2020, 5:58 PM

Rebased. Due to D84182, updated the map types expected by my new tests in clang/test/OpenMP/target_data_codegen.cpp.

This revision was automatically updated to reflect the committed changes.