Page MenuHomePhabricator

[OPENMP50] extend array section for stride (Parsing/Sema/AST)
ClosedPublic

Authored by cchen on Jun 29 2020, 12:22 PM.

Diff Detail

Event Timeline

cchen created this revision.Jun 29 2020, 12:22 PM
ABataev added inline comments.Jun 29 2020, 12:32 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7767–7771

Must be removed

clang/lib/Sema/SemaExpr.cpp
4909

auto->ExprResult

clang/test/OpenMP/target_update_ast_print.cpp
23

It would be good to add some positive tests for other constructs, like target, target data etc., if allowed.

cchen updated this revision to Diff 274209.Jun 29 2020, 12:50 PM

Revert unnecessary changes made by bot

cchen marked 3 inline comments as done.Jun 29 2020, 1:17 PM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7767–7771

I'll remove the commit. Thanks

clang/lib/Sema/SemaExpr.cpp
4909

Should I also change it for LowerBound and Length? Just to make it consistent.

clang/test/OpenMP/target_update_ast_print.cpp
23

I'm going to add those, thanks.

ABataev added inline comments.Jun 29 2020, 1:38 PM
clang/lib/Sema/SemaExpr.cpp
4909

Not in this patch

cchen updated this revision to Diff 274275.Jun 29 2020, 3:55 PM
  1. Remove a commit that is not on master branch
  2. Add a rule in Sema to avoid non-contiguous array section for map clause
  3. Add tests
ABataev added inline comments.Jul 2 2020, 1:42 PM
clang/lib/Parse/ParseExpr.cpp
1927–1942

Better merge into one logical expression.

1933–1934

Same, better to merge into one condition.

clang/lib/Sema/TreeTransform.h
10344

Better to make it this way:

if (Expr *Str = E->getStride()) {
  Stride = getDerived().TransformExpr(Str);
  ...
}
cchen updated this revision to Diff 275241.Jul 2 2020, 2:21 PM

Fix based on feedback

ABataev added inline comments.Jul 2 2020, 2:29 PM
clang/lib/Parse/ParseExpr.cpp
1933

Seems to me, it is too broad. According to the standard, it must be allowed only in to/from clauses.

cchen marked 8 inline comments as done.Jul 6 2020, 10:14 AM
cchen added inline comments.
clang/lib/Parse/ParseExpr.cpp
1933

I did the check in Sema, I'll move the check here. Thanks

cchen marked an inline comment as done.Jul 6 2020, 11:28 AM
cchen added inline comments.
clang/lib/Parse/ParseExpr.cpp
1933

We didn't pass OpenMP clause information here. Do you think I should put the analysis in ParseOpenMPVarList or put the check in SemaOpenMP?

ABataev added inline comments.Jul 6 2020, 12:08 PM
clang/lib/Parse/ParseExpr.cpp
1933

Try to pass it somehow. Maybe, create a parser data member with the current kind of the clause we trying to parse?

cchen updated this revision to Diff 275834.Jul 6 2020, 2:35 PM

Move analysis of stride from Sema to Parse

ABataev added inline comments.Jul 6 2020, 2:48 PM
clang/lib/Parse/ParseExpr.cpp
1944–1951

No, for non-to/from clauses the stride expression should not be allowed at all if I read the standard correctly.

cchen updated this revision to Diff 275856.Jul 6 2020, 4:00 PM

Fix based on comment

ABataev added inline comments.Jul 7 2020, 8:08 AM
clang/lib/Parse/ParseExpr.cpp
1933

You need to insert an additional check for OMPClauseKind == llvm::omp::Clause::OMPC_to || OMPClauseKind == llvm::omp::Clause::OMPC_from here. I.e. we shall expect stride not only if the version is 5.0, but also if the current clauses is to or from

1938–1947

No, what I meant, that ParseExpression() should be called only if OMPClauseKind == llvm::omp::Clause::OMPC_to || OMPClauseKind == llvm::omp::Clause::OMPC_from

cchen marked an inline comment as done.Jul 7 2020, 12:54 PM
cchen added inline comments.
clang/lib/Parse/ParseExpr.cpp
1933

Got it, I was thinking that we might want to emit diagnostic message for OpenMP version < 50. Thanks for your explaination.

cchen marked an inline comment as done.Jul 7 2020, 1:07 PM
cchen added inline comments.
clang/lib/Parse/ParseExpr.cpp
1933

Just want to make sure the error message for OpenMP5.0, for this case: #pragma omp target data map(to: marr[10][0:2:2]).

OpenMP45:
We don't expect stride at all, so we only emit the error message expecting ']' just as before.

OpenMP50:
Should I emit the new error message to inform user that stride can not use in clause other than to or from?

Thanks.

ABataev added inline comments.Jul 7 2020, 1:09 PM
clang/lib/Parse/ParseExpr.cpp
1933

I think, the same behavior just like for OpenMP 4.5 should be fine here since stride is not allowed.

cchen updated this revision to Diff 276589.Jul 8 2020, 3:59 PM

Fix message and rebase

This revision is now accepted and ready to land.Jul 9 2020, 5:13 AM
cchen added a comment.Jul 9 2020, 8:34 AM

@ABataev , can you commit this patch for me when you have time? Thanks.

@ABataev , can you commit this patch for me when you have time? Thanks.

I think you can request commit access from Chris Lattner and commit it yourself.

cchen added a comment.Jul 9 2020, 8:52 AM

@ABataev , can you commit this patch for me when you have time? Thanks.

I think you can request commit access from Chris Lattner and commit it yourself.

I'll do that, thanks for telling!

This revision was automatically updated to reflect the committed changes.