This is an archive of the discontinued LLVM Phabricator instance.

[C++4OpenCL] Introduces __remove_address_space utility
ClosedPublic

Authored by Topotuna on Jul 26 2021, 4:13 AM.

Details

Summary

This change provides a way to conveniently declare types that have
address space qualifiers removed.

Since OpenCL adds address spaces implicitly even when they are not
specified in source, it is useful to allow deriving address space
unqualified types.

Fixes llvm.org/PR45326

Diff Detail

Event Timeline

Topotuna created this revision.Jul 26 2021, 4:13 AM
Topotuna requested review of this revision.Jul 26 2021, 4:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2021, 4:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Change to LanguageExtensions.rst assumes there is a section about the utility on official C++ for OpenCL documentation. However, it is not yet documented

Anastasia added inline comments.Jul 26 2021, 9:32 AM
clang/docs/LanguageExtensions.rst
1970

I think we should migrate this description into here for now and make this a clang-only feature in C++ for OpenCL v1.0. Mainly because new functionality changes can only be added with the version increase. Otherwise, developers would have to know which clang supports it and which doesn't.

But I suggest keeping your draft PR https://github.com/KhronosGroup/OpenCL-Docs/pull/647 open so we don't forget to add it into the next language version doc.

I would like @svenvh to take a look from the clang header's design point.

Topotuna updated this revision to Diff 361937.Jul 27 2021, 1:24 AM

Reference to C++ for OpenCL 1.0 specification removed. More elaborate code example added.

Topotuna marked an inline comment as done.Jul 27 2021, 1:26 AM

I would like @svenvh to take a look from the clang header's design point.

Looks okay to me.

clang/docs/LanguageExtensions.rst
1980
clang/test/CodeGenOpenCLCXX/remove-address-space.clcpp
15

It's maybe worth testing is_same with static_assert(!is_same<float, int>::value); too, or at least testing some negative case.

A case without any address space qualifiers is currently missing (i.e., for the first trait).

Topotuna updated this revision to Diff 361954.Jul 27 2021, 2:44 AM

Additional tests added for is_same

Topotuna marked 2 inline comments as done.Jul 27 2021, 2:45 AM
Anastasia added inline comments.Jul 27 2021, 4:19 AM
clang/docs/LanguageExtensions.rst
1964

Maybe we could title it something like:

Address space removal utility
or
Address space removal trait
?

1968
Anastasia added inline comments.Jul 27 2021, 4:20 AM
clang/docs/LanguageExtensions.rst
1964

Although we could also be more specific and name it
Remove address space builtin function?

Topotuna updated this revision to Diff 361975.Jul 27 2021, 4:38 AM

Documentation rewording

clang/docs/LanguageExtensions.rst
1964

Builtin address space removal utility?

I would prefer not to leave wording "Remove address space" in description because it is already what utility name says and so it adds no extra information.

1968

I would rather use word "affects" instead of "removes" here. There is already excessive amount of "remove" in this paragraph

Topotuna updated this revision to Diff 362027.Jul 27 2021, 7:28 AM

Test case added that checks if __remove_address_space modifies types without address space

The patch looks good but I would like to check with @rjmccall whether it would make sense to add this functionality generically for C/C++ too just like __remove_const and other builtins proposed in https://reviews.llvm.org/D67052?

Anastasia added inline comments.Aug 5 2021, 4:29 AM
clang/docs/LanguageExtensions.rst
1964

Well the section name is to identify the content and not to provide extra information so the closer you are to the actual content the better. I find Remove address space builtin function much more specific to the content to be honest.

Topotuna updated this revision to Diff 364420.Aug 5 2021, 4:46 AM

Documentation section title reworded

Topotuna marked an inline comment as done.Aug 5 2021, 4:46 AM
Anastasia accepted this revision.Aug 5 2021, 7:15 AM

LGTM! Thanks

This revision is now accepted and ready to land.Aug 5 2021, 7:15 AM
This revision was landed with ongoing or failed builds.Aug 6 2021, 2:41 AM
This revision was automatically updated to reflect the committed changes.