- User Since
- Mar 5 2015, 8:05 AM (289 w, 22 h)
Fri, Aug 28
@hans, would you be able to commit this to the release branch?
Thu, Aug 27
This change looks good to me in general conceptually as it is completing missing logic in clang that let's targets to customize the address space behavior!
Wed, Aug 26
Fixed extra space
- Addressed comments from Sven.
Aug 13 2020
Aug 12 2020
Aug 11 2020
Aug 6 2020
Can you extend test test/Preprocessor/macro_variadic.cl to cover C++ for OpenCL mode, otherwise LGTM!
Aug 5 2020
Jul 29 2020
This was also only an initial concept. I think that even once all the issues with the patch have been ironed out, it would require a round of wider review since it's a fairly hefty API change.
Jul 28 2020
LGTM, Thanks! Although I left a comment about possible test simplification that can be addressed before committing.
Jul 27 2020
Jul 24 2020
Jul 20 2020
Jul 17 2020
Jul 13 2020
Jul 10 2020
Jul 8 2020
Jul 3 2020
Jun 29 2020
So if I understand this correctly when the template is instantiated we don't have QualType with the address space deduced initially during parsing of a template global variable?
Jun 26 2020
Jun 23 2020
TODO: update address space numerical values for SPIR target
Regarding the TODO. Still I like an idea of detached address spaces a little bit more, but I see your point. I would love to check how it works with SPIR-V address space mapping in SPIR-V to LLVM IR translator. I know, it shall have a little impact on a clang work, but still what to see the work I have to do there first (5 and 6 are already occupied there).
Jun 19 2020
Btw could you upload a full diff, please?
It will simplify the review....
I think you should also test the conversions of address spaces and it would be good to add a cl file i.e. OpenCL C level test.
Jun 18 2020
Jun 11 2020
LGTM! Thanks! Great spot!
Jun 10 2020
Jun 8 2020
@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?
Jun 4 2020
Seems reasonable from the OpenCL side.
Jun 3 2020
Jun 2 2020
Default address space is normally an address space of automatic storage i.e. private in OpenCL. If you look at the address space map of targets most of them map default and private address spaces to the same value. Converting between private and other named address space is not allowed in OpenCL see section 6.5. I don't believe this change is doing anything legitimate.
Jun 1 2020
LGTM! Thanks! Please address small documentation nitpick before committing...
May 29 2020
May 28 2020
May 22 2020
May 21 2020
May 20 2020
May 19 2020
May 18 2020
Can you please add a reference to the document that described these extensions in the description of your patch. Thanks!
May 10 2020
- Improved behavior by allowing casting between equivalent types.
- Improved formatting.
- Improved tests.
May 6 2020
Sorry for long latency. I have rebased the patch to the current master and addressed the comments from @mantognini too.
May 4 2020
Apr 30 2020
Apr 29 2020
Apr 28 2020
I am looping in @svenvh to discuss this further. Sven, do you think we should rename this flag due to current proposed change? Should it be moved out of -cc1 too?
I would leave -fdeclare-opencl-builtins as it is, until it is complete and taken out of its "experimental" status. Then we can discuss if we want to use TableGen instead of the header as a default.
Would it not become confusing since the builtins are going to be included by default? Should we rename the flag at least? Also ideally it should be documented in https://clang.llvm.org/docs/UsersManual.html#opencl-header
The current solution seems very specific and doesn't provide generic uniform mechanisms for the targets. Would something based on a triple component not work?
I'm somewhat confused by the way this is supposed to work, or why a
separate option exists for OpenCL. The other language flags seem to be
implemented in the driver, not cc1 like OpenCL. I'm not sure this is
the right set of flag naming decisions, or if we really need the
fno-include-default-header cc1 flag.
Apr 14 2020
As for OpenCL C spec we should ideally add __OPENCL_VERSION__ but I feel it should be taken from architecture or OS type... I think there were other cases where we needed something like an OpenCL runtime version in the triple too but I can't remember this now.
Mar 26 2020
Mar 25 2020
Sorry for the delayed comments. It would be good to address those in a separate commit if possible.