This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Added addrspace_cast operator
ClosedPublic

Authored by Anastasia on Apr 3 2019, 4:44 AM.

Details

Summary

This change adds extra cast operator addrspace_cast described earlier in this RFC:
http://lists.llvm.org/pipermail/cfe-dev/2018-December/060546.html

This operator is intended for casting between pointers to objects in different address spaces and follows similar logic as const_cast in C++.

Example:

int* gen = ...;
global int* gl = addrspace_cast<global int*>(gen);

Note that functionality of casting pointers to different address spaces has been previously removed from all other cast operators and can only be currently done using C style casts.

This commit only enables this change for OpenCL, but if agreed it can be changed to apply to C++ too (it needs to be enabled as a valid keyword). However, for C++ it might need to be added as a Clang only extension?

Diff Detail

Event Timeline

Anastasia created this revision.Apr 3 2019, 4:44 AM
Anastasia marked an inline comment as done.Apr 3 2019, 5:04 AM
Anastasia added inline comments.
include/clang/Basic/TokenKinds.def
564 ↗(On Diff #193467)

If I mark it as CXX keyword this will make it available in C++ too because the rest of the patch is not specific to OpenCL but perhaps we also need to emit a warning that this is an extension?

Ping! I would appreciate any feedback to this change please. Thanks you!

When compiling this I get the following warning:

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1213:10: warning: enumeration value 'CXXAddrspaceCastExprClass' not handled in switch

The fix is probably to just add the missing case with the other *CastExprClass cases.

There's also a similar warning for clang/tools/libclang/CXCursor.cpp:132.

include/clang/AST/ExprCXX.h
268 ↗(On Diff #193467)

Maybe the documentation should be updated to include the new cast operator?

475 ↗(On Diff #193467)

The first comma should be removed.

include/clang/Basic/TokenKinds.def
564 ↗(On Diff #193467)

This chunk doesn't apply cleanly to master anymore it seems. This line was also modified.

lib/AST/Expr.cpp
3272 ↗(On Diff #193467)

I would keep this empty line.

lib/Parse/ParseExprCXX.cpp
1325 ↗(On Diff #193467)

The documentation above is really C++ specific. Maybe we should add a comment saying that this cast is not from C++ 5.2p1 to avoid any confusion?

lib/Sema/SemaCast.cpp
236 ↗(On Diff #193467)

Shouldn't this line (and the similar one in the header file) be updated to include address space cast?

279 ↗(On Diff #193467)

Still learning here, so could you/someone tell me if I understood things correctly? :)

The type is "dependent" when templates are involved, right? And, here, when we don't know all the concrete types we defer the checking to a later compiler phase? And that later compiler phase would be implemented by "TreeTransform", right?

285 ↗(On Diff #193467)

The formatting looks a bit funny here.

2319 ↗(On Diff #193467)

Just to make sure I understand things correctly: OpenCL is true when dealing with C or C++ mode for OpenCL, right?

2338 ↗(On Diff #193467)

Wouldn't this limit usage of the cast unnecessarily? I'm thinking this could be transformed to a NOP, which could be beneficial to make (template) code simpler to write.

Anastasia updated this revision to Diff 262479.May 6 2020, 2:36 PM
Anastasia added a subscriber: jeroen.dobbelaere.

Sorry for long latency. I have rebased the patch to the current master and addressed the comments from @mantognini too.

When compiling this I get the following warning:

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1213:10: warning: enumeration value 'CXXAddrspaceCastExprClass' not handled in switch

The fix is probably to just add the missing case with the other *CastExprClass cases.

There's also a similar warning for clang/tools/libclang/CXCursor.cpp:132.

I had to add some more bits to CIndex to make it work!

Anastasia marked 4 inline comments as done.May 6 2020, 2:48 PM
Anastasia added inline comments.
lib/Sema/SemaCast.cpp
279 ↗(On Diff #193467)

Precisely!

285 ↗(On Diff #193467)

I agree, I just matched the style of the old code to keep it coherent. Although perhaps I should rather adhere to the current style?

2319 ↗(On Diff #193467)

Precisely!

2338 ↗(On Diff #193467)

I am not sure what you mean. I have added the test for templates and it caught a bug in lib/AST/Expr.cpp with assert condition.

However, now that I think about this more, I believe we should allow compiling this?

__private int* i;
addrspace_cast<private int*>(i);

Currently it outputs an error.

Thanks for your clarifications and updates. Just one tiny question about a test file, but otherwise LGTM.

clang/test/SemaOpenCLCXX/addrspace_cast.cl
20–25

Does this test anything since it's not instantiated?

lib/Sema/SemaCast.cpp
285 ↗(On Diff #193467)

I don't have a strong opinion on what's best.

2338 ↗(On Diff #193467)

Yes, that's what I meant. (Although I see it's not part of this review so I'm not saying this should be changed now.)

martong removed a subscriber: martong.May 7 2020, 7:23 AM
Anastasia updated this revision to Diff 263085.May 10 2020, 2:38 PM
  • Improved behavior by allowing casting between equivalent types.
  • Improved formatting.
  • Improved tests.
Anastasia marked 3 inline comments as done.May 10 2020, 2:43 PM
Anastasia added inline comments.
clang/test/SemaOpenCLCXX/addrspace_cast.cl
20–25

It tests AST creation and Sema but not after the TreeTransforms. I have now enhanced it.

lib/Sema/SemaCast.cpp
285 ↗(On Diff #193467)

I looked at the other review and I think the preference is to stay with the coding style for the new added code.

2338 ↗(On Diff #193467)

It was an easy change so I have updated the patch. :)

mantognini accepted this revision.May 11 2020, 1:48 AM

LGTM

clang/test/SemaOpenCLCXX/addrspace_cast.cl
20–25

It is now much clearer.

This revision is now accepted and ready to land.May 11 2020, 1:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2020, 4:15 AM
akyrtzi added inline comments.
clang/include/clang-c/Index.h
2057

Hi Anastasia, apologies for not catching this earlier, but libclang is intended to keep a stable ABI and changing the enumerator values breaks libclang's ABI compatibility.

Would you be able to make a follow-up commit and restore the enumerator values to their original values? I would suggest to add CXCursor_CXXAddrspaceCastExpr at the end and assign to it the next available value that is not already taken.

This warrants a revert since it's breaking ABI compatibility for our libclang's users. @Anastasia will you be able to take a look at this soon? I plan on reverting this patch in a couple of weeks if the issue is still unresolved.

hans added a subscriber: hans.Oct 29 2020, 6:03 AM
hans added inline comments.
clang/include/clang-c/Index.h
2057
Anastasia added inline comments.Oct 29 2020, 6:53 AM
clang/include/clang-c/Index.h
2057

I guess it's too late to fix 11.0.0, does it make sense to fix 11.0.1?

hans added a subscriber: tstellar.Nov 2 2020, 1:56 AM
hans added inline comments.
clang/include/clang-c/Index.h
2057

That's @tstellar 's call.

mantognini added inline comments.Nov 9 2020, 2:28 AM
clang/include/clang-c/Index.h
2057

@tstellar, does this require any particular action from my side to be included in 11.0.1?