This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)
ClosedPublic

Authored by jdenny on Jul 21 2021, 4:36 PM.

Details

Summary

This patch implements Clang support for an original OpenMP extension
we have developed to support OpenACC: the ompx_hold map type
modifier. The next patch in this series, D106510, implements OpenMP
runtime support.

Consider the following example:

#pragma omp target data map(ompx_hold, tofrom: x) // holds onto mapping of x
{
  foo(); // might have map(delete: x)
  #pragma omp target map(present, alloc: x) // x is guaranteed to be present
  printf("%d\n", x); 
}

The ompx_hold map type modifier above specifies that the `target
data` directive holds onto the mapping for x throughout the
associated region regardless of any target exit data directives
executed during the call to foo. Thus, the presence assertion for
x at the enclosed target construct cannot fail. (As usual, the
standard OpenMP reference count for x must also reach zero before
the data is unmapped.)

Justification for inclusion in Clang and LLVM's OpenMP runtime:

  • The ompx_hold modifier supports OpenACC functionality (structured reference count) that cannot be achieved in standard OpenMP, as of 5.1.
  • The runtime implementation for ompx_hold (next patch) will thus be used by Flang's OpenACC support.
  • The Clang implementation for ompx_hold (this patch) as well as the runtime implementation are required for the Clang OpenACC support being developed as part of the ECP Clacc project, which translates OpenACC to OpenMP at the directive AST level. These patches are the first step in upstreaming OpenACC functionality from Clacc.
  • The Clang implementation for ompx_hold is also used by the tests in the runtime implementation. That syntactic support makes the tests more readable than low-level runtime calls can. Moreover, upstream Flang and Clang do not yet support OpenACC syntax sufficiently for writing the tests.
  • More generally, the Clang implementation enables a clean separation of concerns between OpenACC and OpenMP development in LLVM. That is, LLVM's OpenMP developers can discuss, modify, and debug LLVM's extended OpenMP implementation and test suite without directly considering OpenACC's language and execution model, which can be handled by LLVM's OpenACC developers.
  • OpenMP users might find the ompx_hold modifier useful, as in the above example.

See new documentation introduced by this patch in openmp/docs for
more detail on the functionality of this extension and its
relationship with OpenACC. For example, it explains how the runtime
must support two reference counts, as specified by OpenACC.

Clang recognizes ompx_hold unless -fno-openmp-extensions, a new
command-line option introduced by this patch, is specified.

Questions:

  • This patch adds an "OpenMP Extensions" section to clang/docs/OpenMPSupport.rst. However, the existing use of the word "extensions" in that document is confusing as it refers to standard features. It should probably be replaced with "features". I'm considering that change for a parent patch. Any objections?

Diff Detail

Event Timeline

jdenny created this revision.Jul 21 2021, 4:36 PM
jdenny requested review of this revision.Jul 21 2021, 4:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2021, 4:36 PM
Herald added a subscriber: sstefan1. · View Herald Transcript

That's strange that we need to ignore delete modifier, I would say that most probably there is a bug in the user's code.

Questions:

Clang currently doesn't support OpenMP 5.1 features unless -fopenmp-version=51. Does it make sense to have an option to enable extensions? Instead of a separate option, we could accept something like -fopenmp-version=51,hold,foo.

I would just add a new options -fopenmp-extension or something like this just toenable all non-standard extensions, better to keep -fopenmp-version as is.

In Clang diagnostics, this patch does not add hold to the list of acceptable map type modifiers because it's an extension. Should it? If there were a command-line option to enable extensions, that would make the path clearer.

Yes, with the extensions enabled it should generate the list of all supported modifiers.

I would propose we prefix these new clauses and such with ompx_.

I would propose we prefix these new clauses and such with ompx_.

+1 here

Thanks for the reviews.

That's strange that we need to ignore delete modifier,

It's not ignored in general: the standard (dynamic) OpenMP reference count is set to 0 (if it isn't already) so that, once hold is not in effect, the data can be unmapped.

I would say that most probably there is a bug in the user's code.

Yes, my understanding of the motivation for this OpenACC feature is to protect users from unbalanced increments and decrements. In structured cases, the assumption is that it's unlikely the user intends to unmap early.

Clang currently doesn't support OpenMP 5.1 features unless -fopenmp-version=51. Does it make sense to have an option to enable extensions? Instead of a separate option, we could accept something like -fopenmp-version=51,hold,foo.

I would just add a new options -fopenmp-extension or something like this just toenable all non-standard extensions, better to keep -fopenmp-version as is.

Works for me. I suppose if we later want a syntax for enabling specific extensions, -fopenmp-extension could become an alias for -fopenmp-extension=all.

In Clang diagnostics, this patch does not add hold to the list of acceptable map type modifiers because it's an extension. Should it? If there were a command-line option to enable extensions, that would make the path clearer.

Yes, with the extensions enabled it should generate the list of all supported modifiers.

Agreed.

I would propose we prefix these new clauses and such with ompx_.

+1 here

That's fine for me, but don't the routines use llvm_omp_?

Should we also have that prefix in various enumerators in the implementation? For example, what does OMP_MAP_HOLD become?

That's fine for me, but don't the routines use llvm_omp_?

That was before we "standardized" ompx_ for OpenMP 5.2.

Should we also have that prefix in various enumerators in the implementation? For example, what does OMP_MAP_HOLD become?

I'd suggest OMPX_MAP_HOLD

That's fine for me, but don't the routines use llvm_omp_?

That was before we "standardized" ompx_ for OpenMP 5.2.

Ah. Thanks.

Should we also have that prefix in various enumerators in the implementation? For example, what does OMP_MAP_HOLD become?

I'd suggest OMPX_MAP_HOLD

Makes sense.

jdenny updated this revision to Diff 361029.Jul 22 2021, 5:08 PM
jdenny retitled this revision from [OpenMP][OpenACC] Implement `hold` map type modifier extension in Clang (1/2) to [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2).
jdenny edited the summary of this revision. (Show Details)

Applied discussed changes plus some additional cleanup:

  • Renamed hold to ompx_hold.
  • Internally, there's OMPC_MAP_MODIFIER_ompx_hold, which is named automatically based on the modifier name. For consistency, I went with OMP_MAP_OMPX_HOLD and OMP_TGT_MAPTYPE_OMPX_HOLD instead of OMPX_MAP_HOLD and OMPX_TGT_MAPTYPE_HOLD. For example, grep -i ompx_hold then finds all of these. It's a quick search-and-replace to switch to the latter if people prefer.
  • Implemented -fopenmp-extensions. Without it, Clang doesn't recognize ompx_hold.
  • Extended the diagnostic that lists expected map type modifiers to include ompx_hold if -fopenmp-extensions.
  • Added phab review numbers to the ompx_hold entry in the OpenMP status page.
  • Wordsmithed new documentation some.
jdenny updated this revision to Diff 365857.Aug 11 2021, 3:05 PM
jdenny edited the summary of this revision. (Show Details)
  • Rebased.
  • Extended docs with motivation section.
ABataev added inline comments.Aug 12 2021, 4:01 AM
clang/include/clang/Driver/Options.td
2382–2385

Use marshalling, if possible

clang/lib/Basic/OpenMPKinds.cpp
64

I would enable this since OpenMP 5.2, since in 5.2 ompx_ is officially allowed extension format.

jdenny added inline comments.Aug 12 2021, 9:32 AM
clang/lib/Basic/OpenMPKinds.cpp
64

Do you mean both -fopenmp-version=52 and -fopenmp-extensions should be required to enable ompx_hold?

Or do you mean only -fopenmp-version=52 should be required to enable ompx_hold, and we can drop the -fopenmp-extensions implementation?

ABataev added inline comments.Aug 12 2021, 9:41 AM
clang/lib/Basic/OpenMPKinds.cpp
64

Second option. Actually, we can enable it if either -fopenmp-version=52 or -fopenmp-extensions is used, depends if we want to add a switch for non-standard OpenMP extensions. If OpenMP 5.2 is on, we can just ignore -f[no]-openmp-extensions. Thoughts?

jdenny added inline comments.Aug 12 2021, 10:13 AM
clang/lib/Basic/OpenMPKinds.cpp
64

Consider that, if -fopenmp-version=52 is sufficient to enable all extensions using the ompx_ prefix, then all such extensions will be enabled by default once OpenMP 5.2 is the default. At that point, won't it be strange that standard OpenMP 5.3 or 6.0 features will be disabled by default while some features appearing in no standard will be enabled by default?

By that logic, it seems -fopenmp-version=52 shouldn't be sufficient to enable extensions. However, it seems overkill to have to specify both -fopenmp-version=52 and -fopenmp-extensions. I think -fopenmp-extensions is a clear enough request to enable the ompx_ prefix regardless of the OpenMP version.

ABataev added inline comments.Aug 12 2021, 10:16 AM
clang/lib/Basic/OpenMPKinds.cpp
64

Ok, let's move with -fopenmp-extensions

jdenny updated this revision to Diff 366057.Aug 12 2021, 12:00 PM

Used marshalling for -f[no]-openmp-extensions, as requested by @ABataev.

jdenny marked 4 inline comments as done.Aug 12 2021, 12:01 PM
jdenny added inline comments.
clang/lib/Basic/OpenMPKinds.cpp
64

OK, thanks.

jdenny marked an inline comment as done.Aug 12 2021, 12:01 PM
ABataev added inline comments.Aug 12 2021, 12:56 PM
clang/docs/ClangCommandLineReference.rst
2042–2044

Default value?

clang/include/clang/Driver/Options.td
2383

Why do you want to disable it by default?

clang/lib/Driver/ToolChains/Clang.cpp
5768–5770

Do we still need this?

5806–5808

Same, do we still need this?

jdenny added inline comments.Aug 12 2021, 1:20 PM
clang/include/clang/Driver/Options.td
2383

I thought that's what we agreed upon.

I'm personally happy if, in general, all features are enabled by default and users opt out by requesting strict conformance, but Clang doesn't do that today even for features from newer OpenMP standards. Should it do so for extensions, which appear in no standard?

What would you prefer?

clang/lib/Driver/ToolChains/Clang.cpp
5768–5770

I tried removing these, but then the driver doesn't pass -fopenmp-extensions to the front end. Am I doing something wrong?

ABataev added inline comments.Aug 12 2021, 1:33 PM
clang/include/clang/Driver/Options.td
2383

I would enable it by default, as it may help users in some cases unless there are no other strong opinions.

clang/lib/Driver/ToolChains/Clang.cpp
5768–5770

Ah, yes, still need to forward the option from driver to the frontend

jdenny updated this revision to Diff 366105.Aug 12 2021, 2:48 PM
jdenny marked 6 inline comments as done.

Addressed @ABataev's suggestions:

  • Enabled extensions by default.
  • Extended the Clang command-line reference to document the default.

I adjusted most tests to pass regardless of the default so there's less work to do if we eventually decide to change it.

jdenny added inline comments.Aug 12 2021, 2:50 PM
clang/include/clang/Driver/Options.td
2383

Well, that promotes my extensions, so I've made that change.

I was wondering about the connection to OpenACC, so I had a quick look into the OpenACC spec to try and understand the background.
OpenACC uses two separate reference counters for structured and unstructured map. If one of them is >0, the data is present. If both become 0, data is deleted.

I think, the hold modifier is not sufficient to replicate OpenACC behavior. Consider the following example:

#pragma acc data copy(a)  // structured ref := 1
{
#pragma acc exit data delete(a) // dynamic ref := 0
#pragma acc enter data copyin(a) // dynamic ref := 1
} // structured ref := 0 // no copyout because dynamic ref >0

As I understand this will be translated to the following OpenMP:

#pragma omp target data map(ompx_hold, tofrom:a)  // ref := 1
{
#pragma omp exit data map(delete:a) // ref := 0  // no action because of hold
#pragma omp enter data map(to:a) // ref := 1
} // ref := 0 // perform map from

I don't think, that trying to map the two openacc reference count to a single openmp reference count will work in general.

I was wondering about the connection to OpenACC, so I had a quick look into the OpenACC spec to try and understand the background.
OpenACC uses two separate reference counters for structured and unstructured map. If one of them is >0, the data is present. If both become 0, data is deleted.

I think, the hold modifier is not sufficient to replicate OpenACC behavior. Consider the following example:

#pragma acc data copy(a)  // structured ref := 1
{
#pragma acc exit data delete(a) // dynamic ref := 0
#pragma acc enter data copyin(a) // dynamic ref := 1
} // structured ref := 0 // no copyout because dynamic ref >0

As I understand this will be translated to the following OpenMP:

#pragma omp target data map(ompx_hold, tofrom:a)  // ref := 1
{
#pragma omp exit data map(delete:a) // ref := 0  // no action because of hold
#pragma omp enter data map(to:a) // ref := 1
} // ref := 0 // perform map from

I don't think, that trying to map the two openacc reference count to a single openmp reference count will work in general.

The next patch in this series (D106510) modifies libomptarget and introduces a second reference count for ompx_hold. There won't be a singe RefCount anymore. I will review that patch once this one has been finalized.

The next patch in this series (D106510) modifies libomptarget and introduces a second reference count for ompx_hold. There won't be a singe RefCount anymore. I will review that patch once this one has been finalized.

Ok, thanks for the clarification. This change was not obvious from the description of the two patches. Makes sense then.

jdenny updated this revision to Diff 366282.Aug 13 2021, 8:01 AM
jdenny edited the summary of this revision. (Show Details)

Updated patch summary to reflect that extensions are now enabled by default.

Adjusted patch summary and the docs to try to clear up the confusion just discussed by @protze.joachim and @grokos. Thanks for raising this point.

This revision is now accepted and ready to land.Aug 13 2021, 10:24 AM

Thanks! I'll wait for the D106510 review before trying to land.

jdenny updated this revision to Diff 367914.Aug 20 2021, 3:13 PM

Rebased.