This is an archive of the discontinued LLVM Phabricator instance.

[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
7770–7774

Must be removed

clang/lib/Sema/SemaExpr.cpp
4907

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
7770–7774

I'll remove the commit. Thanks

clang/lib/Sema/SemaExpr.cpp
4907

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
4907

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
1864–1875

Better merge into one logical expression.

1869–1870

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
1869

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
1869

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
1869

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
1869

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
1880–1887

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
1869

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

1874–1883

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
1869

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
1869

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
1869

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.