This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Parsing and sema support for target update directive
ClosedPublic

Authored by sfantao on Jan 6 2016, 8:44 PM.

Details

Summary

This patch is to add parsing and sema support for target update directive. Support for the to and from clauses will be added by a different patch. This patch also adds support for other clauses that are already implemented upstream and apply to target update, e.g. device and if.

This patch is based on the original post by Kelvin Li.

Diff Detail

Repository
rL LLVM

Event Timeline

kkwli0 updated this revision to Diff 44176.Jan 6 2016, 8:44 PM
kkwli0 retitled this revision from to [OpenMP] Parsing and sema support for target update directive.
kkwli0 updated this object.
kkwli0 added a subscriber: cfe-commits.
ABataev edited edge metadata.Jan 11 2016, 5:32 AM

Kelvin, thanks for the patch!
One more comment: what about nesting of regions? You need to add a corresponding code and tests

include/clang/AST/OpenMPClause.h
3196 ↗(On Diff #44176)

New clauses must be added in separate patches after commit of the new directive.

lib/CodeGen/CGStmtOpenMP.cpp
2685 ↗(On Diff #44176)

Maybe just do not emit anything, to not crash a compiler.

lib/Parse/ParseOpenMP.cpp
71–74 ↗(On Diff #44176)

Probably, we need to add local enumeric for these constants (0, 1, 2 etc.)

kkwli0 marked 3 inline comments as done.Mar 7 2016, 9:32 PM
kkwli0 added inline comments.
include/clang/AST/OpenMPClause.h
3196 ↗(On Diff #44176)

Will do

lib/Parse/ParseOpenMP.cpp
71–74 ↗(On Diff #44176)

Will use the latest infrastructure.

kkwli0 updated this revision to Diff 50024.Mar 7 2016, 9:42 PM
kkwli0 edited edge metadata.
kkwli0 marked 2 inline comments as done.

I rebase the patch to the latest trunk and make use of the infrastructure form parsing and sema.

sfantao edited edge metadata.Mar 9 2016, 10:32 AM

Hi Kelvin,

Thanks for working on this! I have some minor comments inlined.

lib/Sema/SemaOpenMP.cpp
9392 ↗(On Diff #50024)

I'd add an assertion here to make sure CKind is To/Form.

9452 ↗(On Diff #50024)

Same thing.

10045 ↗(On Diff #50024)

Given that these checks are the same, I think it would be better to outline the map clause implementation to some static function and pass the clause kind along. Only a few map-specific things would have to be guarded with the kind.

10108 ↗(On Diff #50024)

You should update the comment. The conflicts are not with the map clause but with to/from clauses of the same constructs. You can still say, that the concerns here are the same as in map clauses so you can reuse the same checks.

10202 ↗(On Diff #50024)

Same thing here.

kkwli0 updated this revision to Diff 50774.Mar 15 2016, 3:12 PM
kkwli0 edited edge metadata.
kkwli0 marked 5 inline comments as done.

Addressed the comments from the last review: added assert calls and outline the common code in ActOnOpenMPToClause, ActOnOpenMPFromClause and ActOnOpenMPMapClause to a static function.

Thanks Kelvin,

I'm fine with the patch. Let's wait for Alexey to see if he has any concern.

Thanks again!
Samuel

ABataev added inline comments.Mar 15 2016, 10:20 PM
include/clang/AST/OpenMPClause.h
3466 ↗(On Diff #50024)

New clauses must be added in separate patches

Thanks for the review.

As request, I will split this patch into two:

  1. to and from clause (without test cases)
  2. target update directive (with test cases)

I will use this review for the target update directive.

Patch for the to and from clauses is in http://reviews.llvm.org/D18488.

sfantao commandeered this revision.May 24 2016, 6:01 PM
sfantao edited reviewers, added: kkwli0; removed: sfantao.
sfantao updated this object.May 24 2016, 6:05 PM
sfantao edited reviewers, added: arpith-jacob; removed: fraggamuffin, rsmith.
sfantao edited subscribers, added: caomhin; removed: arpith-jacob.
sfantao updated this revision to Diff 58368.May 24 2016, 6:10 PM

Add parsing and sema support only for target update and already existing clauses (if and device).

Add a test for nesting of regions here

lib/Parse/ParseOpenMP.cpp
42 ↗(On Diff #58368)

Please, add a comma after 'OMPD_update' to reduce number of changes in the next modification of this enum.

sfantao updated this revision to Diff 58469.May 25 2016, 12:06 PM
sfantao marked an inline comment as done.
  • Add nestings test.
ABataev accepted this revision.May 25 2016, 8:06 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.May 25 2016, 8:06 PM
sfantao closed this revision.May 26 2016, 10:37 AM
This revision was automatically updated to reflect the committed changes.