LGTM! Thanks

I don't feel reinterpret_cast is the right fit for this as it's expected to do just reinterpretation but addrspacecast LLVM instruction which Clang generates might result in some more operations in particular some special instructions for address calculation. Would it work if you enable addrspace_cast with double underscore prefix directly in C++ or something like this? Otherwise C-cast would be just as good as changing reinterpret cast in this way.

LGTM! Thanks

LGTM! Thanks

Committed in https://github.com/llvm/llvm-project/commit/639db8a90d2adf7789b9d3cec3a7c26502bf819b

Updated bullet about nounwind attr

I feel that originally pointers were disallowed because they create the same issue as size_t and etc as their size is implementation depended but the same logic applies to images and other types that are even more implementation depended. Overall this bit of the spec is very inconsistent so I don't mind if we change the behavior to be whatever we find more helpful. However I would encourage to submit an issue to OpenCL-Docs to point out this inconsistency.

It doesn't seem to make sense to have a LangOpt for this, if anything a new CodeGenOpt feels like a better fit. Then you should probably extend existing classify functions to prevent coercion instead of adding yet another function otherwise you will need to handle all the different corner cases again as there seem to be quite many if I look at X86_32ABIInfo::classifyReturnType...

In D141297#4047148, @svenvh wrote:

In D141297#4043122, @Anastasia wrote:

Btw I wonder if in the future we could add some error or warning in case someone uses the same approach for frontend specific features, i.e.

#ifdef __undef___opencl_c_generic_address_space
#error "Feature __opencl_c_generic_address_space can only be disabled via -cl-ext flag"
#endif

Interesting idea, but I'm a bit hesitant of doing so:
It increases the size of opencl-c-base.h so it will take longer to parse, which will affect every OpenCL compilation. Luckily we can avoid that cost if we keep the __undef_ mechanism an internal solution, which it will be once we let -cl-ext=-feature generate __undef_ macros for extensions that are not in OpenCLExtensions.def. So longer term users will never have to pass __undef_ macros.

Btw I wonder if in the future we could add some error or warning in case someone uses the same approach for frontend specific features, i.e.

This doesn't look like a solution. Can you please explain what problem are you trying to solve:

1. Linking multiple LLVM libraries together.
2. Linking multiple LLVM libraries with different LLVM versions?

Any log of error would be helpful too.

Can you link relevant LLVM reviews or documentation and perhaps add some more details into the description?

In D137652#3957519, @svenvh wrote:

@svenvh I remember that we have also discussed the addition of a vendor specific header where such feature/extension macro definition can be added to avoid the macro pollution but I feel this is somewhat orthogonal i.e. the fine grained control of macro defines is still needed?

Unfortunately I don't remember the details of that discussion, but I agree that it's worth looking into a solution for issue #55674, using e.g. __undef macros as you described above.

In D137652#3957233, @FMarno wrote:

@Anastasia I feel like I am following the guidance you quoted here. The defines I've deleted in opencl-c-base.h are blocking the possibility of -cl-ext working and would also get in the way of undefine preprocessor symbols working.
If there is a problem with the additions to OpenCLExtensions.def then we'll need to rework how InitializeOpenCLFeatureTestMacros in initPreprocessor.cpp works.

Are these features affecting the frontend functionality in any way? If not we should implement those in the headers and if headers are not flexible enough to condition out the addition of the new macros we should extend them (potentially by extending behaviour of -cl-ext or introducing undef macros, see https://github.com/llvm/llvm-project/issues/55674). Please review the guidelines here: https://clang.llvm.org/docs/OpenCLSupport.html#opencl-extensions-and-features. The most relevant part here is:

In D134445#3920448, @rjmccall wrote:

In D134445#3920257, @Anastasia wrote:

In D134445#3920188, @rjmccall wrote:

You really can't ask whether a class template pattern is standard layout; it's not meaningful.

Well the templates have to be instantiated with concrete types as kernels can't be called from the host code. But if the concrete type is a reference or pointer there is no full instantiation apparently as AST dumps in the review description show.

My objection here is to the code that does CXXRec = CXXRec->getTemplateInstantiationPattern(). That is the unsubstituted template pattern. You then ask if it has standard layout. I do not think this is the right thing to do.

In D134445#3920188, @rjmccall wrote:

You really can't ask whether a class template pattern is standard layout; it's not meaningful.

Addressed review comments

This makes sense. I believe the same applies to some other languages i.e. OpenCL, as the way clang evolved currently not all targets are compatible with all languages. In the future we might want to generalize this diagnostics to use in other cases too.

Committed in https://github.com/llvm/llvm-project/commit/5e1b36ced538cfc80f2400b27e346d2175b04092.

• Added clarification about the opaque pointers in SPIR-V.

Minor formatting changes

@svenvh as you made quite a lot of changes in the headers feel free to expand the description if you feel we should document some of those in more detail.

CC to @linjamaki in case there is anything else/different needed for HIP.

LGTM! Thanks

LGTM! Thanks

In D127579#3598308, @nikic wrote:

In D127579#3598306, @Anastasia wrote:

In D127579#3595461, @nikic wrote:

In D127579#3588626, @Anastasia wrote:

In D127579#3586092, @nikic wrote:

@Anastasia Thanks, that does sound like a legitimate reason to include the information. I want to double check though, does linking the modules actually fail if the functions have signatures that differ only by pointer types? At least for normal LLVM IR this would work fine, and would just result in the insertion of a bitcast during linking (and then typically the bitcast would get shifted from the called function to the call arguments later).

@nikic If I use spirv-link with two modules that have mismatching pointee type in a function parameter I get an error:

error: 0: Type mismatch on symbol "foo" between imported variable/function %6 and exported variable/function %17.

The way I understand a bitcast instruction in SPIR-V (OpBitcast in https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_conversion_instructions) is that it can only apply to pointer types which are distinct from function types. Note that I believe that function pointers are illegal, at least we disallow them in OpenCL.

Okay ... can we maybe turn this around then? Always emit function parameters as i8* and bitcast them as needed, even if it is possible to guess a better type based on the definition? (Let's ignore the image type case here, which seems to have different requirements from the rest.)

So where would bitcasts be emitted to reconstruct the function prototypes correctly?

The bitcasts would be in the definition only, when the pointer arguments actually get used in a typed manner. The prototype would always include i8* arguments only.

LGTM! Thanks

In D128012#3592126, @beanz wrote:

In D128012#3592098, @Anastasia wrote:

aha, are you having a lot of these types then? Ok that sounds similar problem to OpenCL builtin functions as we had to go away from a simple header include due to long parsing time.

Very similar problem. We have a two-part problem where we both have enough of them that parsing a header would be too slow _and_ the HLSL language actually can't represent them. The later we're working to fix. My hope is that eventually we can write the HLSL libraries in HLSL and use a pre-compilation step (PCH or Module-style) to get past the performance issues.

In D127579#3595461, @nikic wrote:

In D127579#3588626, @Anastasia wrote:

In D127579#3586092, @nikic wrote:

@Anastasia Thanks, that does sound like a legitimate reason to include the information. I want to double check though, does linking the modules actually fail if the functions have signatures that differ only by pointer types? At least for normal LLVM IR this would work fine, and would just result in the insertion of a bitcast during linking (and then typically the bitcast would get shifted from the called function to the call arguments later).

@nikic If I use spirv-link with two modules that have mismatching pointee type in a function parameter I get an error:

error: 0: Type mismatch on symbol "foo" between imported variable/function %6 and exported variable/function %17.

The way I understand a bitcast instruction in SPIR-V (OpBitcast in https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_conversion_instructions) is that it can only apply to pointer types which are distinct from function types. Note that I believe that function pointers are illegal, at least we disallow them in OpenCL.

Okay ... can we maybe turn this around then? Always emit function parameters as i8* and bitcast them as needed, even if it is possible to guess a better type based on the definition? (Let's ignore the image type case here, which seems to have different requirements from the rest.)

Btw do we have any issue tracking this failure, I am having a feeling that the translation of special types might be related to https://github.com/llvm/llvm-project/issues/55121? I am not aware at which point libclc is expected to be linked for SPIR-V but if linking happens at SPIR-V binary level it is likely going to face the other problem I described here.

In D128012#3591828, @beanz wrote:

For just the type alias, adding it during parsing would probably work. Where things get more complicated is we have a whole mess of other data types that we’ll need to add too eventually. One of the advantages of using an external sema source is that we can create incomplete records during initialization, and complete them when the sema source gets the callback from lookup. That gives us cheap and efficient lazy-initialization of our buffer types which (in DXC) has a huge impact on compile time.

I wonder if accepting type alias with identifier vector while parsing would be simpler to implement? Then you could just add those type aliases into the internal header. I assume user code is not allowed to redefine those i.e. it would result in undefined behavior or something like for most of C++ standard library features...

In D127579#3585537, @nikic wrote:

In D127579#3585516, @beanz wrote:

Clang is only permitted to encode pointer type information if that pointer type has some kind of direct semantic meaning -- say, if the pointer element type affects the call ABI in some way. If it does not have direct semantic meaning, then deriving the pointer type based on usage (as DXIL does) and using that for emission would be right approach.

In D127579#3590632, @aeubanks wrote:

If I'm understanding correctly, previously the LLVM pointee type was used to represent a frontend OpenCL type. An alternative to marking everything with elementtype or adding metadata, can something be done in the frontend to mangle the function name with OpenCL pointee types, then the backend translates that to the proper SPIRV types?

In D127579#3588651, @bader wrote:

The way I understand a bitcast instruction in SPIR-V (OpBitcast in https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#_conversion_instructions) is that it can only apply to pointer types which are distinct from function types. Note that I believe that function pointers are illegal, at least we disallow them in OpenCL.

FYI: we are experimenting with function pointers on Intel HW programmed via SPIR-V. Extension draft - https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc.

In D127579#3586092, @nikic wrote:

@Anastasia Thanks, that does sound like a legitimate reason to include the information. I want to double check though, does linking the modules actually fail if the functions have signatures that differ only by pointer types? At least for normal LLVM IR this would work fine, and would just result in the insertion of a bitcast during linking (and then typically the bitcast would get shifted from the called function to the call arguments later).

In D127579#3585537, @nikic wrote:

In D127579#3585516, @beanz wrote:

@nikic the most important thing you need to know about SPIR-V is that it is a virtual ISA based on LLVM IR. The ISA itself encodes types for pointers just like LLVM IR would.

Okay ... I guess my next question would be how the SPIR-V situation differs from the DXIL situation. For DXIL you have already implemented code to "guess" pointer types insofar as they are relevant -- does SPIR-V need something more than that?

Clang is only permitted to encode pointer type information if that pointer type has some kind of direct semantic meaning -- say, if the pointer element type affects the call ABI in some way. If it does not have direct semantic meaning, then deriving the pointer type based on usage (as DXIL does) and using that for emission would be right approach.

The patch seems very straight forward. I wonder though if we should always enable this for SPIR-V w/o adding any flag until the untyped pointers are added to SPIR-V so it will become a target setting controlled instead of a flag?

LGTM! Thanks

ok, so the reason you are adding this to clang is that it needs to be mapped into a target intrinsic?

In D124753#3550670, @python3kgae wrote:

In D124753#3550337, @Anastasia wrote:

In D124753#3546608, @python3kgae wrote:

In D124753#3545779, @Anastasia wrote:

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

This test doesn't seem to correspond to the change being added as you are changing the command-line flags. You don't actually add/generate any attributes in this patch.

Sorry to make things confusing. I should not split default value for -E option as a separate PR :(

There's dxc_E.hlsl (https://reviews.llvm.org/D124751#change-af6Z62NjlfGb) in https://reviews.llvm.org/D124751 where the -E option is added.
dxc_E.hlsl will test -E option translated into -hlsl-entry for cc1.

There was a test for codeGen of -E option in https://reviews.llvm.org/D124752, but I removed it because it is to almost the same as https://reviews.llvm.org/D124752#change-w4NWvaT68Dhk which test codeGen for ShaderAttr.

And the entry_default.hlsl in current PR test codeGen for the default main and -E option.

I can add a separate codeGen test for -E option if that's what you thought is missing.

Ok, makes sense! Thanks!

LGTM! Thanks

In D124753#3546608, @python3kgae wrote:

In D124753#3545779, @Anastasia wrote:

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...

There's already a driver test with '-###' in https://reviews.llvm.org/D124751#change-af6Z62NjlfGb

Please make sure to reflect in your commit message that you are adding the clang internal headers too.

From the current change it seems to me that what you need to be testing is a just that the frontend options are being passed correctly? This should then be a driver test with -### checking for the options to be set for the frontend invocation...