This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP 4.0]Parsing and Sema support for 'omp declare target' directive (accelerator support)
AbandonedPublic

Authored by ABataev on Dec 7 2015, 6:15 PM.

Details

Summary

Add parsing, sema analysis for 'declare target' construct for OpenMP 4.0.
Summary
The declare target directive specifies that variables, functions (C, C++ and
Fortran), and subroutines (Fortran) are mapped to a device. The declare target
directive is a declarative directive.

The syntax of the declare target directive is as follows:
For variables, functions and subroutines:

#pragma omp declare target new-line
declarations-definition-seq
#pragma omp end declare target new-line

Codegen will be done in a separate delivery. This is based on the clang-omp github branch adapted for 3.8.
All unit tests passes (note that message test needed to add -fnoopenmp-use-tls to pass specifically the threadprivate part because use-tls is now set to default). All regression passes.
OpenMP 4.5 changes will be delivered separately.

Diff Detail

Event Timeline

fraggamuffin retitled this revision from to [OpenMP 4.0]Parsing and Sema support for 'omp declare target' directive (accelerator support).
fraggamuffin updated this object.
fraggamuffin added a reviewer: cfe-commits.
fraggamuffin set the repository for this revision to rL LLVM.
fraggamuffin updated this object.
fraggamuffin added a subscriber: cfe-commits.
ABataev edited edge metadata.Dec 7 2015, 10:27 PM

Michael, please provide full diff log, as described in http://llvm.org/docs/Phabricator.html (with full context)

I think we'd better to try to use attributes for declare target. I think it will be much easier and correct solution rather than adding a new declaration

include/clang/AST/DeclBase.h
164 ↗(On Diff #42130)

I don't think this is required, revert it back please

include/clang/AST/DeclOpenMP.h
18–21 ↗(On Diff #42130)

Again, I don't think this change is required. You don't use neither Exprs, nor Types in your changes below

98 ↗(On Diff #42130)

I'm not sure that we need to add this kind of declaration. Most probably it is enough just to have an attribute

include/clang/Basic/DiagnosticSemaKinds.td
7751 ↗(On Diff #42130)

I don't think that OpenMPClauses is suitable warning group for this warning. Maybe you need to add a new one, for example, target specific

include/clang/Basic/OpenMPKinds.h
30 ↗(On Diff #42130)

OMPD_empty is not required, this must be deleted

lib/AST/Decl.cpp
21 ↗(On Diff #42130)

Do you really need this header file here?

lib/AST/DeclBase.cpp
653 ↗(On Diff #42130)

The change is not properly formatted

lib/AST/DeclOpenMP.cpp
10 ↗(On Diff #42130)

comma?

lib/AST/DeclPrinter.cpp
95 ↗(On Diff #42130)

Not formatted

lib/AST/ItaniumMangle.cpp
23 ↗(On Diff #42130)

I don't think this header file is required

lib/AST/MicrosoftMangle.cpp
22 ↗(On Diff #42130)

Again, Are you sure this is required?

lib/CodeGen/CGDecl.cpp
1873–1874 ↗(On Diff #42130)

There is no such object

lib/CodeGen/CodeGenModule.h
1067–1102 ↗(On Diff #42130)

No, this must be deleted. We don't use it here

lib/Parse/ParseDecl.cpp
3624 ↗(On Diff #42130)

Restore original line

lib/Parse/ParseOpenMP.cpp
36–39 ↗(On Diff #42130)

Formatting broken

58–63 ↗(On Diff #42130)

Formatting

80–83 ↗(On Diff #42130)

Formatting

209 ↗(On Diff #42130)

Do not add default:, coding standard recommends to not use it

387–392 ↗(On Diff #42130)

Do not add it

394 ↗(On Diff #42130)

Restore original

lib/Sema/SemaExpr.cpp
13566–13568 ↗(On Diff #42130)

Formatting. Remove braces

lib/Sema/SemaOpenMP.cpp
1139–1140 ↗(On Diff #42130)

I don't understand why you changed this.

1414–1415 ↗(On Diff #42130)

Why you need this declaration if earlier you have a definition?

lib/Serialization/ASTReaderDecl.cpp
353 ↗(On Diff #42130)

Formatting

2411 ↗(On Diff #42130)

Formatting

3307–3309 ↗(On Diff #42130)

Formatting

lib/Serialization/ASTWriterDecl.cpp
134 ↗(On Diff #42130)

Formatting

fraggamuffin marked 14 inline comments as done.Dec 9 2015, 8:26 AM

Michael, please provide full diff log, as described in http://llvm.org/docs/Phabricator.html (with full context)

Ok, no problem. Sorry I forgot. Thanks for the quick review. I only have the other 2 parts to address which is the attribute suggestion and the Helper class which you said should be removed. Thanks again.

include/clang/AST/DeclBase.h
164 ↗(On Diff #42130)

This was there before. I am assuming you just want me to revert back the comma. Done.

lib/Parse/ParseOpenMP.cpp
209 ↗(On Diff #42130)

This is needed to generate these error messages in declare_target_messages.cpp
error: 'error' diagnostics expected but not seen:

File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li

ne 3: unexpected OpenMP directive '#pragma omp end declare target'

File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li

ne 63: unexpected OpenMP directive '#pragma omp end declare target'

File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li

ne 72: unexpected OpenMP directive '#pragma omp end declare target'
3 errors generated.

387–392 ↗(On Diff #42130)

This is needed to prevent infinite loop when there is an unexpected end of the pragma omp target.

lib/Sema/SemaOpenMP.cpp
1139–1140 ↗(On Diff #42130)

The buildDeclRefExpr interface does not enable us to generate these messages.
Someone changed it to buildDeclRefExpr(...) which has a different interface
In the github repository it also uses
ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());

With the buildDeclRefExpr interface, it misses generation of these messages
error: 'warning' diagnostics expected but not seen:

File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li

ne 5: declaration is not declared in any declare target region
error: 'note' diagnostics expected but not seen:

File C:\llvm151206\llvm\tools\clang\test\OpenMP\declare_target_messages.cpp Li

ne 21: used here
2 errors generated.

fraggamuffin edited edge metadata.
fraggamuffin removed rL LLVM as the repository for this revision.
fraggamuffin marked an inline comment as done.

This is just an interim full diff file of changes accepted so far. I still need to address the attribute and Helper class comment. Thanks.

ABataev added inline comments.Dec 14 2015, 7:32 PM
lib/Parse/ParseOpenMP.cpp
209 ↗(On Diff #42130)

Still 'default:' is a bad solution. You must explicitly take all possible cases for the enumeric type

387–392 ↗(On Diff #42130)

As I said before 'default:' is a bad solution. Add explicit cases for all possible values

lib/Sema/SemaOpenMP.cpp
1139–1140 ↗(On Diff #42130)

Github is not a reference version, it has many very bad and not working solutions. You don't need take everything from there, some solutions are just not compatible with trunk

fraggamuffin marked 15 inline comments as done.Dec 29 2015, 10:15 AM

Thanks for the pre-xmas review.

include/clang/AST/DeclOpenMP.h
98 ↗(On Diff #42130)

I added attributes in a few places according to what was done with threadprivate but I am not sure it is enough.

lib/Parse/ParseOpenMP.cpp
209 ↗(On Diff #42130)

I agree and see your point. Thanks. I fixed it by adding the search for end declare target.

387–392 ↗(On Diff #42130)

I agree and see your point. Thanks. I fixed it .

lib/Sema/SemaOpenMP.cpp
1139–1140 ↗(On Diff #42130)

OK, I looked at the uses of build.. and Build.. and see that build... is only used within SemaOpenMP.cpp, so I changed it back, and added my check inside build... for when it is in a target region. Thanks.

fraggamuffin marked 3 inline comments as done.

Fixed Comments from Dec 17.

Hello Michael,
Thanks for working on this. I still think we don't need OMPDeclareTargetDecl here and we can handle everything with an attribute. Try to implement everything just with an attribute and without OMPDeclareTargetDecl.

ABataev commandeered this revision.Jun 16 2016, 2:48 AM
ABataev abandoned this revision.
ABataev edited reviewers, added: fraggamuffin; removed: ABataev.

Revision is abandoned as the construct is supported already.