I've updated the review to include test changes that are required for check-clang-semaopencl to pass.

In D110742#3194608, @svenvh wrote:

Apologies for the late reply... I'd prefer to get the langref updated first, for the sake of consistency and to ensure other stakeholders agree with the interpretation. You can still go ahead with the __attribute__((pure)) changes of course.

In D110742#3048301, @svenvh wrote:

For the constant address space, the const attribute (or readnone) can be used. As memory in the constant address space is immutable, the statement in the langref that: "if a readnone function reads or writes memory visible to the program, or has other side-effects, the behavior is undefined" does not apply. The reading of immutable memory does not have side-effects, nor can it be affected by side-effects.

I think readnone might be too strong, because the pointer argument will still be dereferenced (while readnone implies that "the function computes its result [...] based strictly on its arguments, without dereferencing any pointer arguments").

Unfortunately I cannot look at this in detail right now, but I'll reply quickly for a heads up: The problem was not how to detect kernels, but the ABI/CC mismatch to the clSetKerneArg() with (user facing) arguments that get split to multiple args or vice versa. E.g. x86 ABI had a CC where 2xfloat gets to 1xdouble in the generated function finger print, thus there was no 1:1 match of the kernel arguments to the OpenCL-facing ones (making clSetKernelArg calls difficult to implement robustly).

LGTM, too.

This looks good to me.

If I understand correctly, this needs review from the libc++abi group before I can push it. It's a fairly trivial change. Could someone review this please? (Is there anyone specifically I should add?)

On reflection, I don't think it makes sense to make use of DW_LANG_C_plus_plus_17 or DW_LANG_C_plus_plus_20 in Clang just yet, as these are generally not supported by other tooling. I am a bit confused by DWARF publishing these tags ahead of time, yet tooling having not been updated to recognize them. Regardless, it is clear that support in debuggers needs to come first, which is something that I am not planning to add.

No chances since last time. Rebased to force a rebuild.

Added handling of -gstrict-dwarf and updated tests accordingly.

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Changes will be required to align this with D104291.

This looks good to me.

Add missing trailing commas to CHECK lines of FileCheck test.

Note: there is currently no DWARF language code defined for C++ for OpenCL, so we must use DW_LANG_C_plus_plus* if we wish to be able to determine whether output has been generated from C++ for OpenCL source or from OpenCL C source. I have raised DWARF issue 210514.1 to add a dedicated C++ for OpenCL code in the next version of DWARF, but for now I believe that it is best to use DW_LANG_OpenCL for OpenCL C only, and not for C++ for OpenCL.

In D103833#2810781, @jyknight wrote:

(I note the new behavior also matches gcc)

In D103833#2810319, @jyknight wrote:

but your diff excerpt in the comment above is missing that extra directory prefix. I suspect you've got a local configuration file setting diff.noprefix = true

Add a/ and b/ prefixes to diff.

Reupload with no changes.

@jyknight, I have reuploaded the exact same diff with full context:

Reupload with no changes.

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

Looks good to me.

Looks good, just some suggestions about the test.

Looks good. Small nit about the test case.

This looks like a really good cleanup, in addition to fixing the metadata for pipes.

@hans, is it too late for this to be considered as a candidate for the 9.0.0 release?

In D46015#1078260, @AlexeySotkin wrote:

In D46015#1078235, @stuart wrote:

In D46015#1078217, @AlexeySotkin wrote:

There should not be need for bitcast. Could give an example ? Thanks.

If I have a write_only pipe as the argument to get_pipe_max_packets(), and this uses a single __get_pipe_num_packets() function taking a read_only pipe, we will automatically get a bitcast:

%20 = call i32 bitcast (i32 (%opencl.pipe_ro_t*, i32, i32)* @__get_pipe_max_packets to i32 (%opencl.pipe_wo_t*, i32, i32)*)(%opencl.pipe_wo_t* %19, i32 4, i32 4)

Sorry, but I don't quite understand what does get_pipe_max_packets(), uses __get_pipe_num_packets() mean. Could you clarify? Possibly OpenCL C source example could help.

In D46015#1078217, @AlexeySotkin wrote:

There should not be need for bitcast. Could give an example ? Thanks.

In D46015#1077401, @AlexeySotkin wrote:

It is not clear why we need two versions of get_pipe_num_packets and get_pipe_max_packets builtins. There is only one instruction per builtin in the SPIR-V spec. I think splitting the IR type is enough for translation to SPIR-V purposes.

Changed new getPipeType() method to have protected visibility.