[PATCH] D15125: [OPENMP] 'omp distribute' directive basic support.
Carlo Bertolli via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 2 20:57:13 PST 2015
carlo.bertolli added inline comments.
================
Comment at: include/clang/AST/OpenMPClause.h:667
@@ -666,3 +666,3 @@
OpenMPScheduleClauseKind Kind;
- /// \brief Start location of the schedule ind in source code.
+ /// \brief Start location of the schedule kind in source code.
SourceLocation KindLoc;
----------------
ABataev wrote:
> This must be in a separate patch
OK - I will create a patch dependent on this one for dist_schedule.
================
Comment at: include/clang/AST/OpenMPClause.h:708
@@ -707,3 +707,3 @@
public:
- /// \brief Build 'schedule' clause with schedule kind \a Kind and chunk size
- /// expression \a ChunkSize.
+ /// \brief Build 'dist_schedule' clause with schedule kind \a Kind and chunk
+ /// size expression \a ChunkSize.
----------------
kkwli0 wrote:
> ABataev wrote:
> > I think this is wrong, this is not for dist_schedule, but for schedule clause
> Is it 'schedule'?
Done
================
Comment at: include/clang/AST/OpenMPClause.h:708-709
@@ -707,4 +707,4 @@
public:
- /// \brief Build 'schedule' clause with schedule kind \a Kind and chunk size
- /// expression \a ChunkSize.
+ /// \brief Build 'dist_schedule' clause with schedule kind \a Kind and chunk
+ /// size expression \a ChunkSize.
///
----------------
carlo.bertolli wrote:
> kkwli0 wrote:
> > ABataev wrote:
> > > I think this is wrong, this is not for dist_schedule, but for schedule clause
> > Is it 'schedule'?
> Done
Done
================
Comment at: include/clang/AST/OpenMPClause.h:835
@@ +834,3 @@
+public:
+ /// \brief Build 'schedule' clause with schedule kind \a Kind and chunk size
+ /// expression \a ChunkSize.
----------------
kkwli0 wrote:
> 'dist_schedule'
Done
================
Comment at: lib/Parse/ParseOpenMP.cpp:162
@@ -160,3 +161,3 @@
/// 'for simd' | 'parallel for simd' | 'target' | 'target data' |
-/// 'taskgroup' | 'teams' {clause}
+/// 'taskgroup' | 'teams' {clause} | 'distribute'
/// annot_pragma_openmp_end
----------------
ABataev wrote:
> 'distribute' misses clauses, put your changes right after 'teams'
Add {clause} after distribute, but I see that distribute is already after teams in this comment. Where else should I move it?
================
Comment at: lib/Parse/ParseOpenMP.cpp:670
@@ -666,1 +669,3 @@
DelimLoc = ConsumeAnyToken();
+ } else if (Kind == OMPC_dist_schedule) {
+ Arg = getOpenMPSimpleClauseType(
----------------
kkwli0 wrote:
> Can we merge it with the OMPC_schedule block? The code is similar.
Yes, I also believe that this is the right thing to do as schedule type "static" is the same for dist_schedule and schedule.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1989
@@ -1979,2 +1988,3 @@
// | teams | taskloop | + |
+ // | teams | distribute | !! |
// +------------------+-----------------+------------------------------------+
----------------
ABataev wrote:
> Add rules for distribute to all other directives
Done and added rules for distribute itself when containing other directives.
================
Comment at: lib/Sema/SemaOpenMP.cpp:2026-2027
@@ -2015,3 +2025,4 @@
ShouldBeInOrderedRegion,
- ShouldBeInTargetRegion
+ ShouldBeInTargetRegion,
+ ShouldBeInTeamsRegion
} Recommend = NoRecommend;
----------------
ABataev wrote:
> I don't think this change is required
I added this to specifically express that distribute needs to be closely nested inside teams, but then reverted to do something different. Following your next comment I made changes that made this necessary again. Please check my answer to your next comment and the associated change in the new patch.
================
Comment at: lib/Sema/SemaOpenMP.cpp:2062-2072
@@ -2050,2 +2061,13 @@
return false;
+ if (isOpenMPDistributeDirective(CurrentRegion)) {
+ // OpenMP 4.5 [2.17 Nesting of Regions]
+ // The region associated with the distribute construct must be strictly
+ // nested inside a teams region
+ if (ParentRegion != OMPD_teams) {
+ SemaRef.Diag(StartLoc,
+ diag::err_omp_distribute_strictly_nested_in_teams);
+ return true;
+ }
+ return false;
+ }
if (CurrentRegion == OMPD_cancellation_point ||
----------------
ABataev wrote:
> This must be handled in lines 2173-2181
Those lines (2173-2181) check that teams contains the right directives. It means that we look at the parent and we enter the if-then branch only if the parent region is a teams.
What I am trying to do here is instead check if the parent of any distribute directive is a teams region. If not, I raise an error. This is different form lines 2173-2181 because I need to prove that I am *not* in a teams region.
Anyway, I do agree with you that this should be done elsewhere. I added a new if after the lines you indicated that check the condition above. To report the correct nesting error, however, I needed the ShouldBeInTeamsRegion value above.
Please check the new version of the patch and the regression test for this.
================
Comment at: lib/Sema/SemaOpenMP.cpp:5780
@@ +5779,3 @@
+ if (ChunkSize) {
+ if (!ChunkSize->isValueDependent() && !ChunkSize->isTypeDependent() &&
+ !ChunkSize->isInstantiationDependent() &&
----------------
kkwli0 wrote:
> Is the IsNotNegativeIntegerValue useful in this case?
I think this is related to templates and we are not checking the value of ChunkSize until later (lines 5795-5796). I did not find a IsNotNegativeIntegerValue for the type llvm::APSInt but maybe I should look elsewhere?
I am leaving the code as is for now but please let me know how you think should be done instead.
================
Comment at: tools/libclang/CIndex.cpp:4489
@@ -4477,1 +4488,3 @@
+ case CXCursor_OMPDistributeDirective:
+ return cxstring::createRef("OMPForDirective");
case CXCursor_OverloadCandidate:
----------------
ABataev wrote:
> kkwli0 wrote:
> > "OMPDistributeDirective"
> "OMPDistributeDirective"
Done
================
Comment at: tools/libclang/CIndex.cpp:4489
@@ -4477,1 +4488,3 @@
+ case CXCursor_OMPDistributeDirective:
+ return cxstring::createRef("OMPForDirective");
case CXCursor_OverloadCandidate:
----------------
carlo.bertolli wrote:
> ABataev wrote:
> > kkwli0 wrote:
> > > "OMPDistributeDirective"
> > "OMPDistributeDirective"
> Done
Done
Repository:
rL LLVM
http://reviews.llvm.org/D15125
More information about the cfe-commits
mailing list