Page MenuHomePhabricator

[SYCL] Prohibit arithmetic operations for incompatible pointers
ClosedPublic

Authored by bader on May 20 2020, 12:46 PM.

Details

Summary

This change enables OpenCL diagnostics for the pointers annotated with
address space attribute SYCL mode.

Diff Detail

Event Timeline

bader created this revision.May 20 2020, 12:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 12:46 PM
bader marked an inline comment as done.May 20 2020, 12:49 PM
bader added inline comments.
clang/lib/Sema/SemaExpr.cpp
10089–10090

Alternative approach would be to remove S.getLangOpts().OpenCL to enable this check for all modes.

@Anastasia, do you know if it's safe to do?

Anastasia added inline comments.
clang/lib/Sema/SemaExpr.cpp
10089–10090

If I look at embedded C (ISO/IEC TR 18037) s5.3 rules that we are following largely in Clang I believe this is a universal rule!

Especially looking at the followong statement:

Clause 6.5.6 - Additive operators, add another constraint paragraph:
For subtraction, if the two operands are pointers into different address spaces, the address spaces must overlap.

So I think that it should apply to at least OpenCL, C and C++. I am surprised though that this has not been fixed yet.

I am CCing @rjmccall and @jeroen.dobbelaere in case they have any more insights on this.

rjmccall added inline comments.May 20 2020, 11:38 PM
clang/lib/Sema/SemaExpr.cpp
10089–10090

Yes, I agree, this should apply in all modes.

You should able to restructure the isAddressSpaceOverlapping function so that it can work directly on the address spaces of LHSPointeeTy and RHSPointeeTy; the code will be both cleaner and faster.

bader updated this revision to Diff 265487.May 21 2020, 6:02 AM

Enable diagnostics for non-OpenCL modes and applied refactoring proposed by John.

bader marked 2 inline comments as done.May 21 2020, 8:14 AM

Thanks for the review.
I've enabled the diagnostics for all the modes.
I also applied the refactoring suggested by @rjmccall. Hopefully I understand it correctly.

Please add a C test case just using the address_space attribute.

clang/include/clang/AST/Type.h
1069

It's idiomatic to take QualType by value rather than const &.

Can you rewrite the PointerType method to delegate to this? Assuming it isn't completely dead, that is.

bader updated this revision to Diff 265547.May 21 2020, 11:00 AM
  • Added C test case with address_space attribute.
  • Move isAddressSpaceOverlapping from PointerType to QualType.
  • Move C++ test case to clang/test/SemaCXX
bader marked 2 inline comments as done.May 21 2020, 11:12 AM
bader added inline comments.
clang/include/clang/AST/Type.h
1069

It isn't completely dead, but there were just a few uses of the PointerType method, so I've updated all of them to avoid code duplication in two classes.

rjmccall accepted this revision.May 21 2020, 11:48 AM

LGTM

clang/include/clang/AST/Type.h
1069

Even better, thanks.

This revision is now accepted and ready to land.May 21 2020, 11:48 AM
bader updated this revision to Diff 265579.May 21 2020, 12:34 PM
bader marked an inline comment as done.

Fix formatting in clang/test/Sema/address_spaces.c

This revision was automatically updated to reflect the committed changes.