Page MenuHomePhabricator

[OpenMP] `omp begin/end declare variant` - part 1, parsing
ClosedPublic

Authored by jdoerfert on Feb 20 2020, 5:55 PM.

Details

Summary

This is the first part extracted from D71179 and cleaned up.

This patch provides parsing support for omp begin/end declare variant,
as defined in OpenMP technical report 8 (TR8) [0].

A major purpose of this patch is to provide proper math.h/cmath support
for OpenMP target offloading. See PR42061, PR42798, PR42799. The current
code was developed with this feature in mind, see [1].

[0] https://www.openmp.org/wp-content/uploads/openmp-TR8.pdf
[1] https://reviews.llvm.org/D61399#change-496lQkg0mhRN

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 20 2020, 5:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 20 2020, 5:55 PM

Will tests come in a later patch?

clang/include/clang/AST/OpenMPClause.h
7181–7182

only the -> only on the ?

clang/lib/Parse/ParseOpenMP.cpp
1801

Can there be an else here? Or is this the recommended style?

ABataev added inline comments.Feb 21 2020, 7:21 AM
clang/lib/Parse/ParseOpenMP.cpp
1358–1360

Better to emit a warning here about extra tokens at the end of the directive

Will tests come in a later patch?

I have tests, seems I forgot to add them. Will fix later today with the other things.

jdoerfert marked 2 inline comments as done.Feb 21 2020, 7:58 AM
jdoerfert added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
1358–1360

I think I copied that at some point but I'll add a helper and a warning

1801

I can make it else if

kkwli0 added a subscriber: kkwli0.Feb 21 2020, 1:13 PM
kkwli0 added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1236

Can we merge it with err_expected_end_declare_target?

clang/lib/Parse/ParseOpenMP.cpp
1784

DeviseSetOnly -> DeviceSetOnly

Hi @jdoerfert ,

thank you for working on this.

I have a couple of high level comments.

  1. I am not familiar with OpenMP technical report 8 (TR8). Could you please provide a link in the description? Same for PR42061, PR42798, PR42799. It would be useful to get an idea of the syntax.
  1. It is my understanding that this code cover for functionalities that are still under development in the OpenMP standard. To avoid confusion when reading the code base, I think it would be worth marking the enumerations, the error message and (when possible) the methods with an extra descriptor that marks them as experimental. For example, rename OMPD_begin_declare_variant to OMPD_experimental_begin_declare_variant, or something similar. In the (admittedly unlikely) event the feature doesn't get in future releases of the standard, it will be easier to clean up the code.
  1. This is parsing - why did you have to add also code in the semantic part of the compiler?

Thanks!

Francesco

Since it is a TR8 feature, should we have this guarded by -fopenmp-version=?

Since it is a TR8 feature, should we have this guarded by -fopenmp-version=?

Not this one. It is required to fix the issues with math functions and should work for all versions of OpenMP. Otherwise 4.5 will remain broken.

jdoerfert edited the summary of this revision. (Show Details)Feb 21 2020, 2:32 PM
jdoerfert marked 6 inline comments as done.Feb 21 2020, 2:37 PM
  1. I am not familiar with OpenMP technical report 8 (TR8). Could you please provide a link in the description? Same for PR42061, PR42798, PR42799. It would be useful to get an idea of the syntax.

Added a link in the commit message. Also to some other commit that might be interesting to see that this is a long time coming.
Wrt. the syntax, arguably with this patch clang actually tells you the syntax: https://godbolt.org/z/Cjomow

  1. It is my understanding that this code cover for functionalities that are still under development in the OpenMP standard. To avoid confusion when reading the code base, I think it would be worth marking the enumerations, the error message and (when possible) the methods with an extra descriptor that marks them as experimental. For example, rename OMPD_begin_declare_variant to OMPD_experimental_begin_declare_variant, or something similar. In the (admittedly unlikely) event the feature doesn't get in future releases of the standard, it will be easier to clean up the code.

I have strong reservations against doing that (here). Let me give you the top two:

  1. This will go into 5.1.
  2. Things we name "experimental" are traditionally *never* renamed.
  1. This is parsing - why did you have to add also code in the semantic part of the compiler?

Because I wanted to avoid warnings due to non-exhaustive switch statements :)

clang/include/clang/Basic/DiagnosticParseKinds.td
1236

Did that and forgot to delete it.

jdoerfert updated this revision to Diff 246032.Feb 21 2020, 3:57 PM
jdoerfert marked an inline comment as done.

Add tests and address comments

jdoerfert marked an inline comment as done.Feb 21 2020, 4:00 PM
jdoerfert added inline comments.
clang/test/OpenMP/begin-declare-variant_elided_range_withouth_end.c
5 ↗(On Diff #246032)

The errors referring to clang attribute will be removed from this review again. Accidentally included a SEMA work-in-progress state here.

jdoerfert updated this revision to Diff 246035.Feb 21 2020, 4:09 PM

Revert test check lines to the parser part only

jdoerfert marked an inline comment as not done.Feb 21 2020, 6:52 PM
jdoerfert updated this revision to Diff 246094.Feb 22 2020, 6:27 PM

minor adjustment

Thanks! Splitting this out of D71179, which I think ultimately reached consensus, makes the diff much easier to read.

Subscribing Greg, as I think this is a path to removing a lot of downstream complexity in math.h handling.

aaron.ballman added inline comments.Feb 24 2020, 1:35 PM
clang/include/clang/Parse/Parser.h
2968

This file is inconsistent currently, but it looks like parseOMP... is the current "winner" for the local style (and matches the coding standard). Feel free to rename the other OMP parse functions to be parse instead of Parse as a separate NFC patch, if you'd like.

clang/lib/Parse/ParseOpenMP.cpp
1361

You can drop the hasValue() and just rely on the conversion to bool operator.

1363

You can replace the getValue() calls with ->.

1786

No real value from using this local only once.

jdoerfert marked 5 inline comments as done.

Addressed comments

This revision is now accepted and ready to land.Feb 26 2020, 12:44 PM
This revision was automatically updated to reflect the committed changes.