Could you please clarify the interface to SPIRV-LLVM-Translator tool, specifically:

• Does clang lookup the path to the translator or assume any default path?
• Is there any diagnostic provided if the translator not installed/found?
• How does clang synchronize with the translator versions i.e. some LLVM IR might not be ingested by certain version of the translator. Would this results in the translator ICE or error in the translator, or is it something that can be diagnosed early by clang or during clang build/installation?

LGTM! Thanks

Ping, @Topotuna do you still plan to commit this?

Just to elaborate on the problem that is being solved. We have ended up with cycling dependencies between LangOptions and TargetInfo i.e. some language options became dependent on the target and vice versa.

FYI the original RFC is https://lists.llvm.org/pipermail/cfe-dev/2021-August/068603.html

In D112230#3078231, @azabaznov wrote:

@Anastasia, @yaxunl, do you think it's possible to refactor code generation for blocks such that block literal for global blocks (with no captures) would be emitted in constant address space? Now it's emitted in global address space (for example @__block_literal_global in https://godbolt.org/z/4z8hGj7hz).

In D109144#3066194, @linjamaki wrote:

Thanks for the clarifications. So it seems that you are not expecting that the device target triple is being explicitly passed anywhere then and that means you pass the device triple implicitly?

We are meaning to use the --offload as a way to pass device target triple explicitly.

Although your --offload option does seem conceptually like a device target triple, so I wonder if better naming for it would be --offload-target? Would it work for you if we introduce SPIR-V triple explicitly and you use it as a device offload triple?

To introduce a way to pass a device target triple in HIP compilation, we decided it to be aligned with the envisioned “Unified Offload Option” feature [1] to avoid overlap with similar concept. The design of the feature is pending. AFAIK, @yaxunl, who is working on it, hasn't got time yet to continue the work. In D110622 we propose adding the --offload option as partial implementation and there is a bit of discussion about the design too.

It would probably makes sense to use the same triple to targeting SPIR-V generation by everyone?

Yes, it makes sense.

However I appreciate that OpenCL flow would be somewhat different since it doesn't have a split into host and device but only contains device compilation phase...

The split is specific to offloading languages (such as CUDA, HIP and OpenMP) whose compilation flow is different from the traditional compilation flow. The traditional compilation flow used for non-offloading languages naturally does not have the host/device compilation split.

Cool. I like the direction... In the end it probably makes sense to separate clang compilation flow into more separate steps. This makes it more suitable to non-LLVM based backends or even MLIR towards Clang Intermediate Language. This looks like a fairly good starting point! Thanks!

This direction of creating a common translator tool makes sense to me! Thanks!

In D109144#3063255, @linjamaki wrote:

Can you explain what does this mean

It was trying to clarify a potential misunderstanding of how programs are compiled when HIPSPV is targeted: For HIPSPV, the SPIR-V code generation is done by the clang driver. When we compile HIP programs for HIPCL or the HIPLZ runtime, we issue a single clang command such as this:

clang++ --offload=spirv64 foo.hip -l<hip-runtime> -o foo

With this, the clang driver compiles the device side code to a SPIR-V binary and then compiles host side code, and embeds the SPIR-V binary to the host (fat) binary.

? In the tests I can see the following --offload=spirv64 which does feel like it is specified explicitly that the target is SPIR-V...

For HIPSPV the --offload option is used to specify the device code target but the end result is a host binary (e.g. x86_64) with device code (SPIR-V binary) embedded in it. We need a way to specify the device code target to be other than the currently fixed amdgcn-amd-amdhsa and using the --offload switch is a solution we are suggesting here.

In D110618#3054257, @pekka.jaaskelainen wrote:

I don't feel it is different for OpenCL though... I am not in favour of repeating the same functionality for every language since the requirement will be likely identical. There is no timeline for when this functionality will be dropped so we have to assume that even though temporary it might be a solution for long enough.

If it is likely the SPIR-V backend won’t land soon and it is expected that also other languages might benefit from the calls to the llvm-spirv tool, the functionality to do so would be better placed in a more generally useful place. Do you have suggestions where this functionality could be moved?

I believe it concerns mostly the call to the llvm-spriv (this is sought in PATH, not given via a command line parameter) inside constructEmitSpirvCommand(). This could be extracted to another function in a utility library so it could be accessed by other languages/tools in the future. Does it sound OK?

I would like to explain the position of the OpenCL tooling community and the directions we would like to take with SPIR-V support in LLVM. We believe that SPIR-V triple and target should be added explicitly to LLVM/Clang for the following reasons:

In D109144#3050945, @linjamaki wrote:

> What I have in mind is to continue using SPIR target for now (until SPIR-V back-end is added).

For instance, SYCL compiler emits code for SPIR target and code format is configured via flag.

-emit-llvm changes output file format for regular C++ compilation flow:

clang++ a.cpp -c -o a.o                      # object format by default 
clang++ a.cpp -c -emit-llvm -o a.bc          # LLVM IR format with `-emit-llvm`

Similar approach for HIP device compilation flow:

clang++ -target spir -x hip a.cpp -cuda-device-only -o a.spv                 # SPIR-V format by default
clang++ -target spir -x hip a.cpp -cuda-device-only -emit-llvm -o a.bc       # LLVM IR (aka SPIR) format with `-emit-llvm` if needed

I think this was proposed in RFC. @linjamaki, am I right?

In the RFC we proposed a HIP compilation flow for producing and embedding SPIR-V binary into the host executable. What was not stated in the RFC clearly is that the process is supposed to be carried out without the need for clients to issue explicit commands for producing SPIR-V binaries and then to link them into the final executable separately. D110622 has test cases as examples for this.

In D109144#3045883, @bader wrote:

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?

What I have in mind is to continue using SPIR target for now (until SPIR-V back-end is added).
For instance, SYCL compiler emits code for SPIR target and code format is configured via flag.

-emit-llvm changes output file format for regular C++ compilation flow:

clang++ a.cpp -c -o a.o                      # object format by default 
clang++ a.cpp -c -emit-llvm -o a.bc          # LLVM IR format with `-emit-llvm`

Similar approach for HIP device compilation flow:

clang++ -target spir -x hip a.cpp -cuda-device-only -o a.spv                 # SPIR-V format by default
clang++ -target spir -x hip a.cpp -cuda-device-only -emit-llvm -o a.bc       # LLVM IR (aka SPIR) format with `-emit-llvm` if needed

I think this was proposed in RFC. @linjamaki, am I right?

Ok, is the idea to deprecate this flag once we switch to llc by default then?

In D110618#3039275, @linjamaki wrote:

In D110618#3032899, @Anastasia wrote:

Considering that SPIR-V translation step is also required for other languages would it make sense to add llvm-spirv as a common tool like for example C/C++ linkers and create a bit of common infrastructure? It might be something we can do as a separate step too but it would be good to make sure nothing prevents us from doing this in the future... We should probably also think about the command line interface unification as it probably doesn't make sense for every language to add their own flags for locating SPIR-V translator.

I don’t think it would work out well. llvm-spirv (or other tool relying on LLVM bitcode input) and LLVM may have version differences that cause obscure error cases - like newer LLVM producing bitcode with new features the tools are not expecting and ready for them. The usage of llvm-spirv tool in the HIPSPV tool chain is not intended to be a longer term solution due to this shortcoming.

In D109144#3042014, @bader wrote:

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

Sorry for the delay. I was on vacation last week. I've added my concerns to the discussion in https://reviews.llvm.org/D108621.

Considering that SPIR-V translation step is also required for other languages would it make sense to add llvm-spirv as a common tool like for example C/C++ linkers and create a bit of common infrastructure? It might be something we can do as a separate step too but it would be good to make sure nothing prevents us from doing this in the future... We should probably also think about the command line interface unification as it probably doesn't make sense for every language to add their own flags for locating SPIR-V translator.

It would be good to get closure on this asap.

LGTM! Thanks

LGTM! Thanks

LGTM! Thanks

Feel free to adjust the naming scheme either in the same commit or separately.

In D109874#3011792, @Topotuna wrote:

Both comments addressed. Personally, I would shuffle words around to rename helper function as getDefaultOpenCLPointeeAddrSpace or getOpenCLDefaultPointeeAddrSpace. Although I am not sure if there are some assumed naming conventions in Clang. Originally, I was following the name of getDefaultCXXMethodAddrSpace.

The feature is also C++ for OpenCL specific to let the fast path remain when not utilizing the
address spaces, as this new implementation will be slower than the bitfields that C++ currently
uses, but I hope this can also be optimized in the future if it turns out to be very slow.

LGTM! Thanks

Please, address suggested test simplifications in the final commit if applicable.

this is now on the release branch https://github.com/llvm/llvm-project/commit/9723fc15338e83737d0c5f7cbf415e7f1d9d1ec3

LGTM! Thanks

LGTM! Thanks!

LGTM! Thanks

• Addressed review comments from @Topotuna
• Reordered items to group features and fixes

LGTM! Thanks

Due to the release completion schedule and my planned absences, it is likely that this needs finalizing by the end of the week. Any small changes can still be added later on should you wish to improve something afterwards up until the release branch is frozen. I just want to make sure that we don't miss adding the main content for the release.

@azabaznov and @airlied the main feedback I am looking for is whether I have missed any important patch and whether you agree with the judgement about the completion status.

Cool! Thanks

LGTM! Thanks

LGTM! Thanks

LGTM! Thanks

LGTM! Thanks

CC: @bader @svenvh

Generally LGTM!

In D106343#2974055, @haonanya wrote:

Hi, svenvh and Anastasia. If you approve the patch, could you please submit it?
I don't have permission to do it.

LGTM! Thanks!

In D108360#2962010, @wingo wrote:

@Anastasia is this good to go for you or does this need closer attention?

LGTM! Thanks

LGTM! Thanks

LGTM! Thanks

LGTM! Thanks

This is a very nice clean up!

LGTM! Thanks

In D107769#2960441, @svenvh wrote:

I have done an alternative spin of this patch: instead of introducing RequireDisabledExtension, simply always make the private, global, and local overloads available. This makes tablegen diverge from opencl-c.h though. Perhaps we should also always add make the private, global, and local overloads available in opencl-c.h?

In D108461#2960030, @Topotuna wrote:

__opencl_c_generic_address_space should not have been chosen as the first feature to be addressed out of all OpenCL 3.0 optional core features. This is because __opencl_c_device_enqueue has a dependency on this feature. I will review the order in which OpenCL 3.0 optional features should be introduced to C++ for OpenCL 2021 and return to this revision when all dependencies are resolved.

Do you think we can test this somehow?

LGTM! Thanks

LGTM! Thanks

I imagine you will be adding some tests that will check the correctness of diagnostic wording in the subsequent commits?

LGTM! Thanks

In D108113#2949173, @svenvh wrote:

In D108113#2949141, @Anastasia wrote:

@svenvh I would like to discuss it further with Sven whether it is a good direction or not?

The underlying motivation for adding those aliases seems to be missing from the description.

Personally, I do not see the point of adding aliases to save two characters (you only save the "20" part of "clc++2021"), while adding potential confusion ("is the language called C++ for OpenCL 2021 or C++ for OpenCL 21, or maybe it has something to do with OpenCL 2.1?"). Also they are being added as deprecated aliases, suggesting they might be removed at some point in the future again?

I'd much rather see one canonical way of specifying the language version, instead of adding multiple aliases to do the same thing. But perhaps there is a good reason that I am not aware of?

@svenvh I would like to discuss it further with Sven whether it is a good direction or not?

Let's add a reference to RFC in the description: https://lists.llvm.org/pipermail/cfe-dev/2021-August/068593.html

Can you elaborate the overall design flow please? Particularly, it is not clear why we need to duplicate OpenCL types and builtins at source level for SPIR-V... Is there any way to avoid this?