This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
ClosedPublic

Authored by jdoerfert on Mar 6 2020, 3:19 PM.

Details

Summary
This is the second part loosely extracted from D71179 and cleaned up.

This patch provides semantic analysis support for `omp begin/end declare
variant`, mostly as defined in OpenMP technical report 8 (TR8) [0].
The sema handling makes code generation obsolete as we generate "the
right" calls that can just be handled as usual. This handling also
applies to the existing, albeit problematic, `omp declare variant
support`. As a consequence a lot of unneeded code generation and
complexity is removed.

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].

The logic is as follows:

If we have seen a `#pragma omp begin declare variant match(<SELECTOR>)`
but not the corresponding `end declare variant`, and we find a function
definition we will:
  1) Create a function declaration for the definition we were about to generate.
  2) Create a function definition but with a mangled name (according to
     `<SELECTOR>`).
  3) Annotate the declaration with the `OMPDeclareVariantAttr`, the same
     one used already for `omp declare variant`, using and the mangled
     function definition as specialization for the context defined by
     `<SELECTOR>`.

When a call is created we inspect it. If the target has an
`OMPDeclareVariantAttr` attribute we try to specialize the call. To this
end, all variants are checked, the best applicable one is picked and a
new call to the specialization is created. The new call is used instead
of the original one to the base function. To keep the AST printing and
tooling possible we utilize the PseudoObjectExpr. The original call is
the syntactic expression, the specialized call is the semantic
expression.

[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.Mar 6 2020, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2020, 3:19 PM

There will be an update later today or tomorrow, I am making the actual target offloading CUDA interoperability "work" right now.

jdoerfert planned changes to this revision.Mar 8 2020, 9:36 AM

I'll try to remove the extra namespace and I'll split the function(is_definition) matcher out.

jdoerfert retitled this revision from [OpenMP] `omp begin/end declare variant` - part 2, semantic analysis to [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG").Mar 11 2020, 1:43 PM
jdoerfert edited the summary of this revision. (Show Details)
jdoerfert updated this revision to Diff 249784.Mar 11 2020, 3:37 PM

Direct implementation of the standard wording, replace broken declare variant handling

jdoerfert updated this revision to Diff 249785.Mar 11 2020, 3:41 PM

Update comment

Harbormaster completed remote builds in B48902: Diff 249785.
jdoerfert updated this revision to Diff 249936.Mar 12 2020, 7:51 AM

Minor corrections

Update tests to match new mangling

unconditionally create explicit declarations

hfinkel added inline comments.Mar 24 2020, 3:15 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1253

We're not extremely consistent about how we phrase these kinds of errors, but I grepped for stray and we have only one message about stray tokens, but grepped for matching and we have a bunch more. Phrasing this as:

'#pragma omp end declare variant' with no matching ''#pragma omp begin declare variant'

might be better.

clang/include/clang/Sema/Sema.h
9727

scope

clang/lib/Sema/SemaDecl.cpp
13552

Please add a comment here explaining why the TemplateParameterLists.empty() check is here.

clang/lib/Sema/SemaOpenMP.cpp
5396

Is there any way in which this name might become visible to users (e.g., in error messages)?

5471

Please provide an example or otherwise explain why this failure behavior is correct.

5487

Why is the ordering here useful? Don't you collect all of the variant clauses from all of the declarations? Can't there be duplicates? (and does the relative order need always be the same?) Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the while (CalleeFnDecl) loop, but this makes me wonder if that would still have an odd behavior.

jdoerfert updated this revision to Diff 252485.Mar 24 2020, 8:33 PM
jdoerfert marked 7 inline comments as done.

Rebase and address comments

jdoerfert added inline comments.Mar 24 2020, 9:05 PM
clang/lib/Sema/SemaOpenMP.cpp
5396

Yes, I think so. One way to trigger it would be to define the same function in the same omp begin declare variant scope (or two with the same context). I haven't verified this though.
TBH, I'm unsure how bad this actually is in the short term. The original name is still a at the beginning. We should obviously make sure the error message is appropriate eventually, e.g., it de-mangles the name.

5471

Tried to improve the comment.

5487

Why is the ordering here useful?

I don't think it is ordered. We have a conceptual set and BestIdx determines the which of the set is the best. All others are equal.

Don't you collect all of the variant clauses from all of the declarations?

Yes.

Can't there be duplicates? (and does the relative order need always be the same?)

Yes (and no). getBestVariantMatchForContext should determine the best regardless of duplicates, we might just try it multiple times if we didn't manage to create a call.

Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the while (CalleeFnDecl) loop, but this makes me wonder if that would still have an odd behavior.

We are supporting that. All declarations are scanned and all variants are collected. What odd behavior do you refer to?

hfinkel added inline comments.Mar 25 2020, 8:02 AM
clang/lib/Sema/SemaOpenMP.cpp
5396

I think that we have to consider diagnostic quality as a first-class citizen. There are a few different ways that we might approach this. A minimal diff from what you have might be:

  1. Replace the "." above with ".ompvariant."
  2. Add logic to FunctionDecl::getNameForDiagnostic (or NamedDecl::getNameForDiagnostic) that recognizes that magic string in the name and alters the printing to do something intelligible for the user with the name.

(I think that (1) is a good idea anyway).

Another way would be to add some first-class property to the function and then leave the mangling to CodeGen. Are we mangling these in Sema in other places too?

Also, these can have external linkage, right?

5487

Makes sense, thanks.

jdoerfert marked an inline comment as done.

hide the mangling consistently from the user (and fix rebase bug)

clang/lib/Sema/SemaOpenMP.cpp
5396

I added a test for this in begin_declare_variant_messages.c and confirmed my earlier statement. Then I implemented what you describe above (in spirit).

Another way would be to add some first-class property to the function and then leave the mangling to CodeGen. Are we mangling these in Sema in other places too?

That would potentially work but require us to touch a lot of places that deal with redefinitions and overload resolution. Getting the right clashes between specialized versions with the same name and openmp context but not getting the clashes for the same name but different openmp context will be non-trivial. Similarly, we need to ignore specialized versions during overload resolution, etc. etc.

FWIW, this is design 8 or 9, or even more. I tried to keep the names till codegen in the beginning, using an approach similar to what you described. I tried to use namespaces (even in C) to get the right kind of name clashes and interactions, I tried ... This is the only thing that (so far) worked reliably and is (IMHO) very non-intrusive.

Also, these can have external linkage, right?

Yes. My name clash test has 3 different linkages, see begin_declare_variant_messages.c.

hfinkel added inline comments.Mar 25 2020, 11:51 AM
clang/lib/AST/DeclarationName.cpp
147 ↗(On Diff #252640)

Would it make sense to print " (omp variant)" after the name to disambiguate?

jdoerfert updated this revision to Diff 252712.Mar 25 2020, 5:30 PM

pretty print variant names as "variant[device={kind(cpu)}]"

jdoerfert updated this revision to Diff 252713.Mar 25 2020, 5:36 PM

Split off NFC namespace stuff

hfinkel accepted this revision.Mar 26 2020, 5:13 PM

LGTM

This revision is now accepted and ready to land.Mar 26 2020, 5:13 PM
This revision was automatically updated to reflect the committed changes.
clang/test/OpenMP/begin_declare_variant_messages.c