[SPIR-V] Add SPIR-V triple architecture and clang target info

[linjamaki]

Summary

Add new architectures ‘spirv32’ and ‘spirv64’ in Triple, ported from
LLVM SPIR-V Backend repository
(https://github.com/KhronosGroup/LLVM-SPIRV-Backend). Add basic and
code generation target info for ‘spirv32’ and ‘spirv64’ and, thus,
enabling clang (LLVM IR) code emission to SPIR-V target. The target
information for SPIR-V is mostly reused from the SPIR target info as
starter.

Clang code generation output is mostly same as SPIR. Added and updated
tests for parts that are different between SPIR and SPIR-V now.

[Anastasia]

Generally LGTM!

I only have one suggestion regarding testing - I feel that it makes more sense to just omit the tests that are not specific to the target itself. This is mainly because SPIR-V is now inherited from SPIR so the same logic applies in most of places. I think it would make sense that we only add tests for the logic which is not identical to SPIR. For example, tests that verify the target properties and target specific macros, etc make sense to keep but most of the tests that are just checking the language semantic using SPIR triple are not going to be different for SPIR-V (at least not now). So we can slowly build up relevant testing in the subsequent patches and therefore avoid unnessasary test time increase.

clang/lib/Basic/Targets.cpp

609

I wonder how complete is the support of logical addressing SPIR-V triple? It seems like you don't test it in the clang invocation at the moment and it is therefore missing from TargetInfo.

Do you have plans to implement it in the subsequent patches? If not it might be better to leave it out for now.

[Anastasia]

CC: @bader @svenvh

[linjamaki]

Remove unused triple, reduce tests and fix a lint error. Other lint errors seems to be out of my control.

[Anastasia]

clang/test/CodeGenOpenCL/spirv32_target.cl

12 ↗

(On Diff #370541)

Are these lines tested somehow? You could change this into C++ for OpenCL test and use static_assert or find some other ways to test this...

However, this testing seems to overlap with the lines at the end... Could you please elaborate on the intent of this test?

Also if you don't plan this to be fundamentally different from testing of 64bit triple I think this should be merged with spirv64_target.cl. It will make things easier for the maintenance and further evolution.

[linjamaki]

clang/lib/Basic/Targets.cpp

609

I removed the spirvlogical triple. AFAIK, this triple has zero sized pointers and I don't know if the LLVM is ready for that yet.

clang/test/CodeGenOpenCL/spirv32_target.cl

12 ↗

(On Diff #370541)

I used spir{32,64}_target.cl as a base for checking codegen for SPIR-V with the only difference being the triple check. The lines give an compile error (e.g. error: 'res1' declared as an array with a negative size) if the sizeof operators give unexpected result.

I'll merge this file with the spirv64_target.cl.

[linjamaki]

Merge two tests together.

[Anastasia]

clang/test/CodeGenOpenCL/spirv32_target.cl

12 ↗

(On Diff #370541)

Ok, you could consider adding -verify and

//expected-no-diagnostics

Even though FileCheck should not succeed but it is going to be more clear that some functionality is tested via diagnostics for the future modifications.

However, you seem to be testing pointer size in the two lines below which also seem to be tested via //CHECK directives... not sure whether both are needed.

[Anastasia]

LGTM! Thanks.

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

I am also not sure if more types should be tested from the data layout... but my guess is that testing could be improved in the subsequent commits.

[linjamaki]

Add -verify and expected-no-diagnostics to a SPIR-V test.

[linjamaki]

Hi @Anastasia, could you submit this patch for me (I don’t have commit access)? Thanks.

[linjamaki]

Rename SPIRABIInfo -> CommonSPIRABIInfo and SPIRTargetCodeGenInfo -> CommonSPIRTargetCodeGenInfo

[Anastasia]

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

[bader]

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.

[Anastasia]

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.

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.

Thanks! Just to summarize here current discussions, the following approaches for the SPIR-V support in Clang are being discussed:

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? We provide documentation and wider communication in the community that SPIR and SPIR-V are the same target and will be evolved the same way. This would mean that if we do require modifications that are not backward compatible the old ABI might be broken and tools using SPIR would have to adapt. Or if we are to avoid such modifications we would not be able to make a significant improvement to SPIR-V if there are any.
2. Create SPIR-V target as a subclass of SPIR deriving most of the common functionality but leaving the ability to deviate freely. This is essentially what this patch implements. The concern is the code duplication although that should be minimal considering that C++ inheritance and metaprogramming features can be used to effectively share the code.

I think to unblock this thread we should request further feedback from the community.

• What architectural changes are needed/desired for SPIR-V tooling in LLVM and whether they can be done uniformaly for SPIR and SPIR-V.
• What is the envisioned tooling evolution for SPIR and SPIR-V and could we safely consider that they are aligned?

[bader]

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?

[Anastasia]

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?

I would find it slightly confusing that the target spir will produce SPIR-V binary, but the confusion can be improved with some good docs although probably not resolved completely. Do you then suggest that we add SPIR-V triple and target for the SPIR-V backend once it is available in LLVM tree?

I believe we are not too far from enabling initial support of SPIR-V backend in LLVM so I thought the goal was to add SPIR-V target asap that can be used universally for both the translator and the backend...

[linjamaki]

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

[Anastasia]

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.

Can you explain what does this mean

without the need for clients to issue explicit commands for producing SPIR-V binaries

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

[Anastasia]

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:

• It would be better in the future to get away from the use of SPIR because SPIR has been discontinued by the Khronos Group and ever since it has evolved in various ways that are not documented anywhere to our knowledge. The use of SPIR for SPIR-V has largely been a workaround due to the failure to add SPIR-V target natively in Clang/LLVM. We would like to be able to deprecate or even remove SPIR in the future if the community is happy to switch to SPIR-V fully. Therefore we believe that it is better for the LLVM project not to create any new uses of it but instead teach the developer community to use SPIR-V target straight away.
• We believe that adding SPIR-V explicitly will better align with the conventional architecture of Clang/LLVM where each distinct target has its corresponding triple/target representation. It will also eliminate confusion in the developer community as both SPIR and SPIR-V exist in the specification as completely different formats and both are supported in tooling.
• With wider adoption of SPIR-V support in LLVM (i.e. backend or even MLIR) adding a new triple seems to be inevitable so it is better to add it straight away in order to avoid rewriting code later on or force application developers to switch to a different interface.
• We would like to improve and optimize the design of the LLVM stack for SPIR-V - for example, we are planning to change the design of the OpenCL builtins for SPIR-V that we won't be able to change for SPIR as it is used by other tools (for example clspv) differently. We can't force vendors to change their implementations because of our goals. This would be impractical considering how long SPIR was used in tools around OpenCL and possibly other languages too.

Note that we are aware that switching to SPIR-V target would require some changes in tools including SPIRV-LLVM Translator that we find fully justifiable and we, therefore, don't have any objections to adding such changes.

[linjamaki]

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.

[Anastasia]

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.

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? 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? It would probably makes sense to use the same triple to targeting SPIR-V generation by everyone? 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...

[linjamaki]

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.

[linjamaki]

Rebase.

[Anastasia]

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.

Ok, thanks for the details. It seems we all at least need a separate triple for SPIR-V even though it might not be called exactly a conventional triple... However, making a distinct target that corresponds to the triple is conventional...

[linjamaki]

Is this patch ready to land on the LLVM? We do not have commit rights, so can you please commit this patch for us? Thanks.

[Anastasia]

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