Page MenuHomePhabricator

[OpenMP] Context selector extensions for template functions
ClosedPublic

Authored by jdoerfert on Aug 11 2020, 7:48 AM.

Details

Summary

With this extension the effects of omp begin declare variant will be
applied to template function declarations. The behavior is opt-in and
controlled by the extension(allow_templates) trait. While generally
useful, this will enable us to implement complex math function calls by
overloading the templates of the standard library with the ones in
libc++.

Diff Detail

Event Timeline

jdoerfert created this revision.Aug 11 2020, 7:48 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
jdoerfert requested review of this revision.Aug 11 2020, 7:48 AM

Nice. What makes it an extension? 5.0 / 2.3.5 claims "and where variant-func-id is the name of a function variant that is either a base language identifier or, for C++, a template-id." which suggests this could be always-on

clang/lib/Sema/SemaOpenMP.cpp
5916

tabs!

Nice. What makes it an extension? 5.0 / 2.3.5 claims "and where variant-func-id is the name of a function variant that is either a base language identifier or, for C++, a template-id." which suggests this could be always-on

This is for begin/end declare variant, a 5.1 feature. There it says:

The begin declare variant directive associates the context selector in the match clause with each function definition in declaration-definition-seq.

So it applies to function definitions between the begin and end. With the extension it applies to template function definitions too.

Rebase on top of D85878

Test formatting

JonChesterfield accepted this revision.Aug 25 2020, 4:13 PM

This looks good to me. I can't claim to have read the test case in detail. Some style suggestions inline around the vector, but they're not blocking.

clang/include/clang/Sema/Sema.h
10042

SmallVectorImpl => SmallVector?

clang/lib/Sema/SemaOpenMP.cpp
5871–5876

Return SmallVector<FunctionDecl> by value instead of via the reference? Makes it clear that we aren't adding to an existing vector

5916

This turned out to be how phabricator represents changes in indent. Unfortunate that the symbol is the one I usually see used for tabs.

clang/test/AST/ast-dump-openmp-begin-declare-variant_template_2.cpp
58

I'd like a different way for us to test this stuff. Realistically impossible to spot errors in 200 lines of tree. Don't have a suggestion at this time.

This revision is now accepted and ready to land.Aug 25 2020, 4:13 PM
ABataev added inline comments.Aug 25 2020, 4:39 PM
clang/include/clang/Sema/Sema.h
10042

It is a good practice to pass SmallVectors as SmallVectorImpl to make it independent of concrete SmallVector template specialization.

jdoerfert marked 5 inline comments as done.Aug 25 2020, 4:43 PM
jdoerfert added inline comments.
clang/include/clang/Sema/Sema.h
10042

We usually use the Impl versions for the APIs to avoid hardcoding the "size" everywhere.

clang/lib/Sema/SemaOpenMP.cpp
5871–5876

Hm, I guess we can do that, it will cause some more copies. FWIW, we are adding to an existing vector if bases is not empty. I mean, if you provide an empty vector we add and if there is something in the vector we add.

clang/test/AST/ast-dump-openmp-begin-declare-variant_template_2.cpp
58

I manually checked it, and now we can verify changes. At the end of the day checking this by hand is actually not too bad. The trick is that you can skip most of it. You go to the test function return value at the very end. Each call expression there should call a function that returns 0. So follow the callee and check the return there (nothing else anyway). I used this scheme for all tests and once you did verify one it is really easy to do the others. Assuming my input is proper ;)

This revision was landed with ongoing or failed builds.Wed, Sep 16, 11:40 AM
This revision was automatically updated to reflect the committed changes.
jdoerfert marked 2 inline comments as done.