Friendly ping^2.

Ping.

Add an option (dislabed by default) to strip CU with references to non-existing const global expressions.

In D131179#4235319, @aprantl wrote:

I think your example makes perfect sense, but it sounds like this should be a feature that only llvm-extract or whatever tool you are using in the described workflow should enable. Generally you don't want "dead" constant's debug info stripped in a normal compilation because you still want to be able to see these constants in the debugger. In your workflow the compiler has special knowledge that this debug info will be available externally, so it makes sense there.

LGTM. Thanks!

Ping.

@hvdijk, thanks a lot for fixing this.

LGTM. Thanks!

"[SPIR-V] Add support for arithmetic_fence builtin for SYCL targets." -> "[SPIR] Add support for arithmetic_fence builtin for SPIR target."

@beanz, could you help with reviewing this change, please?

Should we generalize and rename clang/test/CodeGenOpenCL/convergent.cl to validate function attributes other than convergent? It's not obvious that presence of nounwind attribute is validated by clang/test/CodeGenOpenCL/convergent.cl.

I expect this to be a common issue for all single-source offloading programming models (i.e. CUDA and HIP in addition to SYCL and OpenMP offload). Probably we can generalize the code patterns used in this patch for all of them.

I can understand the reason for changes in llvm/lib/ExecutionEngine/ExecutionEngine.cpp and tests, but the purpose of changes in llvm/lib/ExecutionEngine/Interpreter/ is not clear. Could you elaborate more on that, please?

@jcranmer-intel, thanks a lot for working on this. I'm so excited to see these changes!
Overall, it looks good to me, but I'd like to avoid some runtime computations if possible.

Thanks for the fix!

Is binary size a concern here? NVIDIA, AMD and Intel GPUs are already have ~ 20 different architectures each, so I want my app/library to run on any GPU from these vendors (which is quite reasonable expectation), I'll need to have/distribute ~ 60 different binaries. libdevice, libm, libc are quite small, but other apps (e.g. ML frameworks) might be quite large, so that distributed binary size is important to consider.

Update commit message.

@urnathan, sorry, we didn't notice that https://reviews.llvm.org/D130909 introduces divergence. @steffenlarsen, @jcranmer-intel, FYI.
Regarding dropping LLVM_ prefix from header guards, it looks like these were removed with some automatic tool (https://github.com/intel/llvm/commit/c13a09a462807936f9eb17cc64f53ad7c9e8ddec), so I would expect someone else will run the tool again and add them back. Any ideas how to prevent this type of changes in the future? Maybe we can keep LLVM_ prefix for sources in llvm/ directory and just ignore this difference. What do you think?

In D131179#3784293, @aprantl wrote:

I don't think this would end up matching the user expectation. If I'm in the debugger I would expect to be able to access global constants in the expression evaluator.

@aprantl, thank you a lot for reviewing this patch. Could you also take a look at the part moved to https://reviews.llvm.org/D131179, please?

Ping.

Ping.

Apply clang-format. No functional changes.

@aprantl, sorry for the delay. I'd like to finish this patch on behalf of @mlychkov.
I've applied all your suggestions. Please, take a look.

Applied code review suggestions.

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.

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.

LGTM. Thanks!

LGTM, just one suggestion.
It would be great to get @aaron.ballman approve too.

In D115708#3192495, @tstellar wrote:

LGTM. I'm not sure the best place to document this.

In D110622#3174113, @tra wrote:

The patch looks OK for the time being. That said, I do have concerns that we may be organically growing something that will be troublesome to deal with long-term.

TBH, I still can't quite make sense of where/how SPIR-V fits in the offloading nomenclature.

Right now we have multiple levels of offloading-related control points.

• offload targets, specified by --offload-arch. Determines the ISA of the GPU binary we produce.
• offload mechanism: OpenMP, CUDA runtime, HSA. Determines how we compile/pack/launch the GPU binaries.
• front-end: CUDA/HIP/ C/C++ w/ OpenMP.
• Driver: Determines compilation pipeline to glue everything together,

SPIR-V in these patches appears to be wearing multiple hats.
It changes compilation pipeline, it changes offload mechanism and it changes offload targets.

In D109818#3169531, @linjamaki wrote:

The patch is ready to land. @Anastasia, @bader, could you commit this patch to the LLVM for us? Thanks.

LGTM, with a couple of minor suggestions.

@tra, ping.
@steffenlarsen, does it make sense to add support for recently released 11.5 as well?

This part looks good to me. Just a couple of minor style comments.

@vlastik, your commit fixes function pointers on AVR - https://github.com/llvm/llvm-project/commit/57fd86de879cf2b4c7001b6d0a09df60877ce24d. I suppose this change is required for fixing lvalue references to function pointers on AVR as well. Right?

In D71016#3063762, @tschuett wrote:

Would a codegenSYCL directory help you to separate Sema from code generation?

In D71016#3063457, @tschuett wrote:

It feels like you are doing codegen(OpenCL kernel) in Sema. Are OpenCL kernels the only approach.

In D109144#3042247, @Anastasia wrote:

1. Implementing SPIR-V target as SPIR target. @bader do you suggest that we add spirv triple to clang and map it into SPIR taget or do you suggest something different?

In D109144#3032865, @Anastasia wrote:

It would be good to get closure on this asap.

@bader We had related discussions on the other reviews about the approach in this patch. If you have any concerns/suggestions can you please notify asap...

LGTM, just one typo in addition to linter reports and I'd like John to take a look.

LGTM.