https://reviews.llvm.org/rGdae9fe408793def8a49f5e1d10d2a859627785e3

@hans, would you be able to commit this to the release branch?

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!

Fixed extra space

• Addressed comments from Sven.

In D62574#2210503, @ebevhan wrote:

I think this works now. It should probably be given a few more reviewers to have a look. Do you have some suggestions, @Anastasia ?

LGTM! Thanks

Can you extend test test/Preprocessor/macro_variadic.cl to cover C++ for OpenCL mode, otherwise LGTM!

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.

In D62574#2136423, @ebevhan wrote:

It seems that D70605 attempted to ameliorate the issues that I observed (pointer-conversion doing too much), but it didn't manage to solve the problem fully.

LGTM, Thanks! Although I left a comment about possible test simplification that can be addressed before committing.

LGTM! Thanks

LGTM! Thanks!

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?

LGTM! Thanks!

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).

Btw could you upload a full diff, please?
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
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.

In D68994#2073827, @nhaehnle wrote:

In D68994#2060451, @PiotrFusik wrote:

What is the status of this change?

I still want to pursue a proper modeling of convergent that would probably express that the invalid optimizations that you're likely to see are forbidden. However, the model described here has some issues, and I hope to introduce a slight variation that addresses those issue.

As a matter of pragmatism, I wonder whether we shouldn't just already disable those problematic optimizations today...

LGTM! Thanks! Great spot!

In D80932#2083185, @bader wrote:

In D80932#2080631, @Anastasia wrote:

@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?

I can't answer this question for the reasons I have explained above.

Sorry, I'm not sure that I get your concern correctly, so let me clarify: is it allowing conversion between pointers w/ and w/o address space annotation in non-SYCL mode or using OpenCL address space attributes in SYCL mode?

Just to help you to understand the proposed design, I created the full patch for address space handling in SYCL: https://github.com/bader/llvm/pull/18. There are few CodeGen tests validating LLVM IR for SPIR target. Fee free to ask any questions on this PR.

@Anastasia, if we make this change specific to SYCL mode, will it address your concerns?

In D80932#2073542, @Naghasan wrote:

In D80932#2072064, @Anastasia wrote:

In D80932#2071321, @bader wrote:

In D80932#2068863, @Anastasia wrote:

Why? Can you explain what you are trying to achieve with this?

I think @asavonic can provide more detailed answer, but IIRC we spent a lot time trying to marry template meta-programming with OpenCL address space deductions, but even simplest template functions from C++ standard library breaks compilation. It's important to note one important difference in design goals for SYCL and C++ for OpenCL. SYCL aims to enable compilation of regular C++ as much as possible where as one of the C++ for OpenCL goals is to keep compatibility with OpenCL C. These two goals might lead to different design decisions.

I don't see a contradiction in those goals. We plan to support C++ libraries with C++ for OpenCL of course with functionality that is accepted by conformant OpenCL implementations. For example, libraries that use virtual functions leading to function pointers are not going to work portably in OpenCL devices. If SYCL aims to compile to conformant OpenCL devices then it should not be very different I imagine?

(Edited a bit)

There is a fundamental difference (given the direction taken for OpenCL), OpenCL mode actively modifies user's types (C and C++ for OpenCL AFAIK). So inside a function, this:

int var;

becomes

VarDecl  var '__private int'

Which is fine in C where you can't really do much with your types, but this has a massive and destructive effect in C++. For instance this does not compile in C++ for OpenCL:

void foo() {
    int var; // '__private int' not 'int'
    int* ptr1 = &var; // '__generic int* __private' not 'int*'
    decltype(var)* ptr2 = ptr1; // error: cannot initialize a variable of type 'decltype(var) *__private' (aka '__private int *__private') with an lvalue of type '__generic int *__private'
}

Seems reasonable from the OpenCL side.

In D79781#2070998, @PiotrFusik wrote:

Great! Thanks! Feel free to go ahead and commit this!

Thanks. Can someone commit this for me, please? Also to 10.0 if possible?

In D80932#2071321, @bader wrote:

In D80932#2068863, @Anastasia wrote:

Why? Can you explain what you are trying to achieve with this?

I think @asavonic can provide more detailed answer, but IIRC we spent a lot time trying to marry template meta-programming with OpenCL address space deductions, but even simplest template functions from C++ standard library breaks compilation. It's important to note one important difference in design goals for SYCL and C++ for OpenCL. SYCL aims to enable compilation of regular C++ as much as possible where as one of the C++ for OpenCL goals is to keep compatibility with OpenCL C. These two goals might lead to different design decisions.

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.

LGTM! Thanks! Please address small documentation nitpick before committing...

In D79781#2063440, @PiotrFusik wrote:

Bumped the OpenCL version from 1.2 to 2.0.

LGTM! Thanks!

In D80416#2050250, @arsenm wrote:

The langref wording makes me think this isn't quite right. This depends on your definition of floating point contraction. I've always assumed it meant allow FMA, potentially increasing precision. Is contracting into something less precise allowed?

Can you please add a reference to the document that described these extensions in the description of your patch. Thanks!

• Improved behavior by allowing casting between equivalent types.
• Improved formatting.
• Improved tests.

In D60193#1549806, @mantognini wrote:

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.

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

In D78979#2015214, @arsenm wrote:

In D78979#2010739, @Anastasia wrote:

In D78979#2010527, @svenvh wrote:

In D78979#2007643, @Anastasia wrote:

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

ah I guess if we leave it under -cc1 we will have the command line interface as follows:

• Driver (without -cc1) adds OpenCL header by default that can be overridden by the flag added in this patch.
• Frontend (with -cc1) doesn't add the header by default but there are two flags -fdeclare-opencl-builtins or -finclude-default-header that allow to include the header.

The name of -fdeclare-opencl-builtins becomes a bit non-intuitive, a more intuitive name would be something like -fopencl-tablegen-builtins perhaps or -fopencl-fast-builtins if we don't want to mention TableGen. But since it is still an experimental option, I have no objections against postponing the renaming until the option has reached maturity.

I vote for -fopencl-fast-builtins.

The main problem I'm trying to solve is that users shouldn't need to concern themselves with including any header, much less a choice between two internal implementation details. Switching the two internal header implementations seems OK to leave as a cc1 flag. The main question I think is what the spelling of how to disable whatever default header was chosen, and what the internal flags to switch the implementation look like.

So what's the agreement on spelling? Use -nostdinc/-nostdlib? Have 2 different cc1 flags for the header implementation switch? Surface/invert -fno-include-default-header?

In D78979#2011582, @jvesely wrote:

In D78979#2006901, @yaxunl wrote:

In D78979#2006847, @arsenm wrote:

I'm also wondering if using -nogpulib for the corresponding linker purpose was correct, since in the OpenCL case it's not really an offload target. Maybe this should switch to -nostdlib?

-nogpulib is fine since opencl compiler is in parallel with the device compiler of CUDA/HIP. The library it uses is the device library.

OpenCL can target other devices than GPUs, including CPUs and FPGAs, referring to gpulibs wrt opencl is a misnomer.

It would be nice to have some clarity as to how OpenCL is handled wrt clang frontend vs. clang driver.
OpenCL options are currently split between the two (e.g. cl-denorms-are-zero is only available in the driver and not the frontend)
There are 3 implementations of CL headers, two in clang which might or might not be included by default, and the 3rd one in libclc.

In D78979#2007760, @yaxunl wrote:

In D78979#2007643, @Anastasia wrote:

ah I guess if we leave it under -cc1 we will have the command line interface as follows:

• Driver (without -cc1) adds OpenCL header by default that can be overridden by the flag added in this patch.
• Frontend (with -cc1) doesn't add the header by default but there are two flags -fdeclare-opencl-builtins or -finclude-default-header that allow to include the header.

Is my understanding correct? We should reflect this is the User Manual.

I think -finclude-default-header -fno-include-default-header better becomes both driver and -cc1 option since users are likely to use it with driver. It may also be used with other languages e.g. HIP.

In D78979#2010527, @svenvh wrote:

In D78979#2007643, @Anastasia wrote:

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

ah I guess if we leave it under -cc1 we will have the command line interface as follows:

• Driver (without -cc1) adds OpenCL header by default that can be overridden by the flag added in this patch.
• Frontend (with -cc1) doesn't add the header by default but there are two flags -fdeclare-opencl-builtins or -finclude-default-header that allow to include the header.

The name of -fdeclare-opencl-builtins becomes a bit non-intuitive, a more intuitive name would be something like -fopencl-tablegen-builtins perhaps or -fopencl-fast-builtins if we don't want to mention TableGen. But since it is still an experimental option, I have no objections against postponing the renaming until the option has reached maturity.

In D78979#2007563, @Anastasia wrote:

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

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.

LGTM! Thanks!

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.

In D76636#1941590, @rjmccall wrote:

In D76636#1941575, @Anastasia wrote:

In D76636#1940086, @rjmccall wrote:

Address spaces are actually defined in a number of standardized C extensions that are compatible with C++, such as the Embedded C specification, OpenCL, and so on. There's precedent for libc++ accepting work that supports this kind of C++-compatible extension in the past; for example, std::is_pointer looks through Objective-C's ARC type qualifiers. That said, (1) libc++ hasn't actually added new traits for such things before, just taught existing traits to handle them, and (2) I think this definition wouldn't actually work for cases like the OpenCL address spaces because they're not expressible as __attribute__((address_space(N))). (2) is fixable; (1) may be a reasonable place to draw the line on policy.

Regarding (2) (although it's probably not changing the major concerns) this exact implementation also works for OpenCL because we map the address space keywords to the attributes during the parsing. However, OpenCL currently doesn't support libcxx. :(

Ah, okay. Maybe it's one of the other languages that keeps its address spaces separate from address_space.

Regardless, if libc++ wants to have a policy of not adding new features that cover non-C++ features, that's a reasonable stance.

Sorry for the delayed comments. It would be good to address those in a separate commit if possible.

In D76636#1941260, @ldionne wrote:

I agree with @EricWF here. Libc++ shouldn't become a repository of useful but non-standard utilities. It's useful to sometimes extend standard tools to non-standard concepts (like handling some Objective-C qualifiers when we can), but adding something new that hasn't been standardized is not really the business we want to be into.

However, @Anastasia , you could consider proposing the attribute for standardization, and if that makes the cut, then we'll be happy to implement it.

Cheers!

In D76636#1940086, @rjmccall wrote:

Address spaces are actually defined in a number of standardized C extensions that are compatible with C++, such as the Embedded C specification, OpenCL, and so on. There's precedent for libc++ accepting work that supports this kind of C++-compatible extension in the past; for example, std::is_pointer looks through Objective-C's ARC type qualifiers. That said, (1) libc++ hasn't actually added new traits for such things before, just taught existing traits to handle them, and (2) I think this definition wouldn't actually work for cases like the OpenCL address spaces because they're not expressible as __attribute__((address_space(N))). (2) is fixable; (1) may be a reasonable place to draw the line on policy.