In D130766#3687240, @Anastasia wrote:

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

From my point of view this patch LGTM.

Sounds good to me. I’ll make a new patch when there is a need for it and close this one.

Thanks for the review, @yaxunl. Could you push this to the LLVM? And to the LLVM 14 release branch too, if possible?

In D117137#3269365, @yaxunl wrote:

Does that mean only "spirv{64}-unknown-unknown" is acceptable, or "spirv{64}-amd-unknown-unknown" is also acceptable?

SPIR-V target requires that the OS and the environment type is unknown (see TargetInfo::AllocateTarget and BaseSPIRTargetInfo). The clang would fail to create a SPIR-V target if there is an OS or environment component in the target string known by the Triple. This could lead to a misleading error message.

This patch should be ready to land. @tra, could you please commit it to the LLVM for us? Thanks.

Rebase.

Thanks, @yaxunl.

Assuming that this patch is ready to land. @tra or @yaxunl, could you please commit this patch to the LLVM for us? Thanks.

If I understand correctly, the default optimization level is -O2 for the OpenCL and OpenCL++ language mode (according to CompilerInvocation.cpp:getOptimizationLevel()). Since higher than -O0 may not work with the translator, should the SPIR-V tool chain override this default and set it to -O0 (unless a user passes some -O option)?

Rebase.

Thanks, @tra.

This patch should be ready to land. @tra, could you please commit this to the LLVM for us. Thanks.

Rebase.

Thanks, @yaxunl.

D109818 includes the change in this patch.

Assuming this patch is ready to land. @yaxunl, Could you please commit this patch to the LLVM for us. Thanks.

Rebase.

Thanks, @bader.

Thanks for the feedback. The --offload is meant to support multiple targets but right now it is restricted to one at most. The limitation comes from the HIPActionBuilder and CudaActionBuilderBase which currently expects a single target triple and toolchain for all offload devices. To relax the --offload target count cap we would need to adjust the action builders to support multiple target triples and create a separate toolchain for each (unique) target triple.

• Add comments to clarify the limitation of the --offload option to one target.

Rebase.

Rebase.

Cherry-picking should work now, @bader.

Rebase.

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

Thanks, @Anastasia.

@tra, gentle ping.

@Anastasia, Could you please commit this patch to the LLVM for us? Thanks.

Disable debug info generation for device code.

In D110618#3148501, @Anastasia wrote:

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?

Thanks for the review.

Thanks for the review.

Retry push changes.

Update a driver test case.

Combine options with append().

Thanks for the help!

• Adjust --offload description: reflect the current state.
• Adjust enum range check in IsAMDGpuArch().
• Make --offload and --offload-arch options mutually exclusive.

Gentle ping.

Gentle ping.

Ping?

Hi @Anastasia and @bader,

Rebase.

Rebase.

Rebase.

Rebase.

Rebase.

Rebase.

Rebase, add asserts and move address space map reset for HIP from SPIRVTargetInfo to BaseSPIRTargetInfo

Rebase.

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.

Rebase on updated llvm-spirv tool (D112404).

Thanks for the review.

Rename SPIRV::Translator's tool names as suggested by bader.

Update for changes in D112404.

Remove --spirv-ext and --spirv-max-version.

Do I read it correctly that you would have no objections for others to rework this patch if needed? :)

Improve --offload option description.

Gentle ping. Is anything needed to be addressed to get this patch accepted?

Gentle ping.

Gentle ping. Is anything needed to be addressed to get this patch accepted?

Thanks for the review. Updated the patch with the style suggestions.

Rephrase expressions.

Rebase.

Rebase.

Rebase.

Rebase and use SPIRV::constructTranslateCommand() to contruct
the LLVM-SPIR-V translation command.

Rebase.

Rebase.

Rebase.

Rebase.

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.

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.

> 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 D110685#3044993, @Anastasia wrote:

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

Repurpose 'Generic' CudaArch, Use getAllArgValues() for reading
--offload values and fix a enum range.

Update option description.

In D110622#3030792, @tra wrote:

A Cuda GPU architecture ‘generic’ is added. The name is picked from the LLVM SPIR-V Backend. In the HIPSPV code path the architecture name is inserted to the bundle entry ID as target ID. Target ID is expected to be always present so a component in the target triple is not mistaken as target ID.

How generic is 'generic'? If I understand the statement above correctly, it should probably reflect that it's specific to spir-v.
If it's the only possible spir-v variant, then calling it`spir-v` might be more meaningful.
If we expect to see other spir-v variants in the future it would allow us to clearly differentiate between them later.
E.g. --offload=spirv-a,spirv-b. It would be rather odd if we had to use --offload=generic, spirv-b.

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.