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.

Diff Detail

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
7617

What about binary operator?

clang/lib/Sema/SemaOpenMP.cpp
15703

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

15717

Again, bad idea to count this stuff.

15734

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
7617

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
15734

Will fix this.

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

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

15717

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
10116

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
7617

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

clang/lib/Sema/SemaOpenMP.cpp
15703

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.

15717

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
15717

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
15688

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

15713–15715

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

15719–15723

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
7634

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

clang/lib/Sema/SemaOpenMP.cpp
15719–15723

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
15688

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
15688

(*(..)) 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
15688

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
10116

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

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7617–7634

Do you have the tests for these changes?

7750

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

clang/lib/Sema/SemaOpenMP.cpp
15688

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.

15700–15701

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

15708–15711

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

15720–15722

Just return Visit(RE); without else

15723

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
10116

Will fix this, thanks.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7617–7634

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
15695

Hmm, addr_of creates rvalues.

15698

No need for else here

15715

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

15725

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
15715

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
15715

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
15715

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
15689–15690

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
15689–15690

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
10117

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
15500–15502

Shall we do this only if RelevantExpr is nullptr?

15512–15513

Shall we do this only if RelevantExpr is nullptr?

15573–15574

Shall we do this only if RelevantExpr is nullptr?

15681–15685

Shall we do this only if RelevantExpr is nullptr?

15712

Shall we do this only if RelevantExpr is nullptr?

15717

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

15718

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
15500–15502

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
15500–15502

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
15500–15502

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.