This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP5.0] Allow pointer arithmetic in motion/map clause
ClosedPublic

Authored by cchen on Feb 24 2020, 1:29 PM.

Details

Summary

Base declaration in pointer arithmetic expression is determined by
binary search with type information. Take "int *a, *b; *(a+*b)" as an
example, we determine the base by checking the type of LHS and RHS. In
this case the type of LHS is "int *", the type of RHS is "int",
therefore, we know that we need to visit LHS in order to find base
declaration.

Event Timeline

cchen created this revision.Feb 24 2020, 1:29 PM
Herald added a project: Restricted Project. · View Herald Transcript
cchen updated this revision to Diff 246293.Feb 24 2020, 1:39 PM

Remove redundant code in test.

ABataev added inline comments.Feb 24 2020, 1:43 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7616

What about binary operator?

clang/lib/Sema/SemaOpenMP.cpp
15698

Better to discuss the implementation for this in a separate patch, it really requires some additional analysis and extra work.

15712

Again, bad idea to count this stuff.

15729

No need for this->

cchen marked 4 inline comments as done.Feb 24 2020, 1:52 PM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7616

Didn't add check here since I didn't add any binary operator into Component in the MapBaseChecker class. Should I add it?

clang/lib/Sema/SemaOpenMP.cpp
15729

Will fix this.

cchen added inline comments.Feb 24 2020, 1:52 PM
clang/lib/Sema/SemaOpenMP.cpp
15698

Why should we do this in another patch? The only thing this patch does is extending for pointer arithmetic (doing analysis on BinOp).

15712

Will it be better if just check if the subtree is an offset? So that we only need to check if it does not have any decorator in type?

ABataev added inline comments.Feb 24 2020, 2:11 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9965

I would say that this lvalue must be addressable, no? Also, some function calls can be handled, probably, those returning rvalues, for example, or constexprs.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7616

I think so. Otherwise those binary expressions are meaningless. Or treated in some unexpected way.

clang/lib/Sema/SemaOpenMP.cpp
15698

You're doing some checks in this patch using counting of '*' and '[' in this patch. This is definitely not correct. That's why I suggest to move it in a separate patch and discus the solution there and do not block all other kinds of expressions.

15712

What do you mean? Exclude, say, RHS or LHS from the analysis, if it is a complex expression and you can use another part as a base? That's worth trying, at least.

cchen updated this revision to Diff 246304.Feb 24 2020, 2:17 PM

Remove the counting in MapBaseChecker, instead, comparing the type of root with
the type of LHS and RHS, and only visit the subtree having the same type as root.
This scheme pass all the test I have. I'll add BinOp into components in the next
revision. Thanks

cchen marked an inline comment as done.Feb 24 2020, 2:20 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15712

Generally yes, but the current implementation was the opposite way: visit the subtree with the same type as root (binop), if found, then we just visit it.

ABataev added inline comments.Feb 25 2020, 7:08 AM
clang/lib/Sema/SemaOpenMP.cpp
15683

I think you need to extend this to support paren expressions, at least.

15708–15710

I don't think you need these variables here anymore.

15714–15718

This does not look correct. At least, it should return the result of one of Visit() calls. Plus, it seems to me, it won't handle something like *(a+5). That's why I suggest excluding binary op from this patch, it is not complete.

cchen updated this revision to Diff 246495.Feb 25 2020, 10:08 AM

Add BinaryOperator in components and fix logic in MapBaseChecker.

cchen marked 6 inline comments as done.Feb 25 2020, 10:13 AM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7629

I'm now not checking for isLValue here since the binary expression without unary operator is not an LValue.

clang/lib/Sema/SemaOpenMP.cpp
15714–15718

Now return the result of Visit() calls. Also, this patch can handle *(a+5) or more complicated pointer arithmetic expression, such as CK6 and CK13 in target_update_codegen.cpp.

cchen marked 5 inline comments as done.Feb 25 2020, 10:18 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15683

I have UO->getSubExpr()->IgnoreParenImpCasts() here, or I might not understand what you mean by "support paren expression".

ABataev added inline comments.Feb 25 2020, 10:22 AM
clang/lib/Sema/SemaOpenMP.cpp
15683

(*(..)) is not supported, I think. Or, maybe, *((a)+b)

cchen marked an inline comment as done.Feb 26 2020, 8:35 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15683

Both cases are handled correctly. For (*((b)+a)), the binop in VisitBinaryOperator()does not include the outer paren and for *((a)+b), the parens can be handled by Expr *SubE = UO->getSubExpr()->IgnoreParenImpCasts();.

ABataev added inline comments.Feb 26 2020, 10:13 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9965

Fix this message, please, it is not quite correct.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7616–7633

Do you have the tests for these changes?

7745

Change this line to just cast<FieldDecl>. In this case the assert is not required.

clang/lib/Sema/SemaOpenMP.cpp
15683

I think we need to change the way we check the expression in call for MapBaseChecker::Visit in checkMapClauseExpressionBase or extend the check in the checkMapClauseExpressionBase. E->IgnoreParenImpCasts() may drop implicit lvalue-to-rvalue cast and we may allow mapping of rvalues.

15695–15696

I think, here we should have return RelevantExpr || Visit(UO->getSubExpr()) without dropping parens and implicit casts. Again, we may drop some important implicit casts

15703–15706

Add a check here that the type of the expression must be a pointer type.

15715–15717

Just return Visit(RE); without else

15718

Unreachable code

cchen marked 2 inline comments as done.Feb 26 2020, 11:55 AM
cchen added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
9965

Will fix this, thanks.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7616–7633

I think target_update_codegen.cpp has already tested those.

ABataev added inline comments.Feb 26 2020, 12:34 PM
clang/test/OpenMP/target_update_codegen.cpp
371–372

I suppose you missed some checks here, where IDX_EXT and TWO are used.

cchen updated this revision to Diff 247022.Feb 27 2020, 10:15 AM

Fix based on feedback

ABataev added inline comments.Feb 27 2020, 11:42 AM
clang/lib/Sema/SemaOpenMP.cpp
15690

Hmm, addr_of creates rvalues.

15693

No need for else here

15710

Not sure this should be an assert. What if you have something like *((a+b) + c) and a+b is not pointer arithmetic?

15720

No need for else here

clang/test/OpenMP/target_update_codegen.cpp
374

Must be just [[TWO]]?

cchen marked 2 inline comments as done.Feb 27 2020, 11:48 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15710

Good point, I'll fix it. Thanks

clang/test/OpenMP/target_update_codegen.cpp
374

You mean use a more representable name?

cchen marked an inline comment as done.Feb 27 2020, 11:58 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15710

Well, for this case, the type of binop expr *((a+b) + c) is int *, and the type of LHS is int, while the type of RHS is int *, therefore, expr (a+b) is not going to be visited. I'm using assert here since before the checkMapClauseExpressionBase be called, we have already check for whether the expression is lvalue. Maybe I'll just error out if binop here is not a pointer type instead of assert?

ABataev added inline comments.Feb 27 2020, 12:25 PM
clang/test/OpenMP/target_update_codegen.cpp
374

No, I mean that here you should not redefine the variable but check that the previously defined TWO is used in the check line.

ABataev added inline comments.Feb 27 2020, 12:27 PM
clang/lib/Sema/SemaOpenMP.cpp
15710

Yes

cchen updated this revision to Diff 247073.Feb 27 2020, 12:44 PM

Fix test and add check for unary operator.

cchen updated this revision to Diff 247075.Feb 27 2020, 12:48 PM
cchen marked 14 inline comments as done.

Remove redundant else

cchen marked 6 inline comments as done.Feb 27 2020, 12:49 PM
ABataev added inline comments.Feb 27 2020, 1:18 PM
clang/lib/Sema/SemaOpenMP.cpp
15684–15685

UO_Deref is always r-value. So, !UO->isLValue() will be always triggered for UO_Deref. You can just exclude it from the condition.

clang/test/OpenMP/target_update_codegen.cpp
375

Seems to me, L is not used in the test, so you can just use {{%.+}} here

ABataev added inline comments.Feb 27 2020, 1:27 PM
clang/lib/Sema/SemaOpenMP.cpp
15684–15685

Oops, sorry, wrong comment, thought about Addr_of, but you removed it already.

cchen updated this revision to Diff 247095.Feb 27 2020, 1:31 PM

Fix based on feedback

Also, would be good to add more tests for the situations that should not be handled, like just (a+b), explicit casts, etc.

clang/include/clang/Basic/DiagnosticSemaKinds.td
9966

I still would change this message to something like expected addressable lvalue in '%0' clause, where %0 must be one of map|to|from to be more specific and correct.

cchen updated this revision to Diff 247101.Feb 27 2020, 1:43 PM

Add back the check of UO_Deref

cchen added a comment.Feb 27 2020, 1:45 PM

Add back the check of UO_Deref

Haven't added tests for some prohibited test cases.

cchen updated this revision to Diff 247122.Feb 27 2020, 3:18 PM

Add test

ABataev added inline comments.Feb 28 2020, 9:25 AM
clang/lib/Sema/SemaOpenMP.cpp
15499–15501

Shall we do this only if RelevantExpr is nullptr?

15510–15511

Shall we do this only if RelevantExpr is nullptr?

15570–15571

Shall we do this only if RelevantExpr is nullptr?

15676–15680

Shall we do this only if RelevantExpr is nullptr?

15707

Shall we do this only if RelevantExpr is nullptr?

15712

Shall we also here do return RelevantExpr || Visit(LE);?

15713

Shall we also here do return RelevantExpr || Visit(RE);?

cchen marked an inline comment as done.Feb 28 2020, 9:36 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15499–15501

Yes, I'm assuming that RelevantExpr is nullptr before those assignments, so maybe adding assert is sufficient?

ABataev added inline comments.Feb 28 2020, 9:39 AM
clang/lib/Sema/SemaOpenMP.cpp
15499–15501

I think assert will crash for something like x?y:z.

ABataev added inline comments.Feb 28 2020, 9:48 AM
clang/lib/Sema/SemaOpenMP.cpp
15499–15501

Though, currently, we just return false in this case. Yes, assert should be enough currently probably.

cchen updated this revision to Diff 247298.Feb 28 2020, 10:00 AM

Add assert to make sure that we never overwrite RelevantExpr

cchen updated this revision to Diff 247303.Feb 28 2020, 10:15 AM

return RelevantExpr || Visit(LE);

cchen marked 13 inline comments as done.Feb 28 2020, 10:16 AM
This revision is now accepted and ready to land.Feb 28 2020, 11:03 AM

@ABataev, can you land it for me when you have time?

Thanks,
Chi Chun

@ABataev, can you land it for me when you have time?

Thanks,
Chi Chun

Sure

This revision was automatically updated to reflect the committed changes.