Page MenuHomePhabricator

[OpenCL] Added addrspace_cast operator
Needs ReviewPublic

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

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

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

475

The first comma should be removed.

include/clang/Basic/TokenKinds.def
564

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

lib/AST/Expr.cpp
3272

I would keep this empty line.

lib/Parse/ParseExprCXX.cpp
1325

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

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

279

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

The formatting looks a bit funny here.

2319

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

2338

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.