Page MenuHomePhabricator

[Driver] Add CUDA support for --offload param
ClosedPublic

Authored by dcastagna on Jan 12 2022, 11:55 AM.

Details

Summary

The --offload option was added in D110622 to "override the default
device target". When it landed it supported only HIP.
This CL extends that option to support SPIRV targets for CUDA.

Diff Detail

Event Timeline

dcastagna created this revision.Jan 12 2022, 11:55 AM
dcastagna requested review of this revision.Jan 12 2022, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2022, 11:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tra added a reviewer: tra.Jan 12 2022, 12:09 PM
tra added a comment.Jan 12 2022, 12:14 PM

I think instead of setting the triple directly from the command line, we should start with adding another --cuda-gpu-arch (AKA --offload-arch) variant and derive the triple and other parameters from it.

jlebar removed a reviewer: jlebar.Jan 12 2022, 4:25 PM
jlebar added a subscriber: jlebar.

I defer to Art.

dcastagna updated this revision to Diff 401461.Jan 19 2022, 5:41 PM
dcastagna retitled this revision from [Driver] Add a flag cuda-device-triple to [Driver] Add CUDA support for --offline param.
dcastagna edited the summary of this revision. (Show Details)

Using already existing --offload parameters instead of adding a new one

I think instead of setting the triple directly from the command line, we should start with adding another --cuda-gpu-arch (AKA --offload-arch) variant and derive the triple and other parameters from it.

As discussed via IM, I uploaded a patch that extends the command line options --offload instead of introducing a new flag.
@tra, what do you think?

tra added a comment.Jan 20 2022, 10:54 AM

LGTM in general, modulo few nits.
Nit: looks like the changes need some clang-formatting.

clang/lib/Driver/Driver.cpp
123–124

You either need to rephrase the diag message in clang/include/clang/Basic/DiagnosticDriverKinds.td and remove the argument, or provide the "CUDA" or "HIP" as the argument. Passing a "" as is will result in an incomplete sentence.

157–158

Just return llvm::Triple("amdgcn-amd-amdhsa") ?

The title says --offline option, which should be --offload.

clang/include/clang/Driver/Options.td
1146

There are other offloading toolchains e.g. OpenMP or SYCL. This option only supports CUDA and HIP, so it is better to add "(CUDA and HIP only)".

dcastagna updated this revision to Diff 401759.Jan 20 2022, 1:10 PM
dcastagna marked an inline comment as done.
dcastagna retitled this revision from [Driver] Add CUDA support for --offline param to [Driver] Add CUDA support for --offload param.

Address tra@ and yaxunl@ comments.
Error out if offload is used without --emit-llvm

dcastagna updated this revision to Diff 401760.Jan 20 2022, 1:13 PM
dcastagna marked 2 inline comments as done.

Address yaxunl@ comment in Options.td

The title says --offline option, which should be --offload.

Fixed that and addressed the other comments.
The last patch also adds a check to error out if --offload is used in CUDA without --emit-llvm, since that will result in a assert failing later on.

dcastagna updated this revision to Diff 401813.Jan 20 2022, 4:21 PM

Fix invalid-offload-options.cpp test

Rebase on ToT

tra added inline comments.Jan 24 2022, 1:41 PM
clang/lib/Driver/Driver.cpp
154–156

What's expected to happen if someone specifies spirv64-nvidia ?

If someone uses spirv64-foo-bar I think it would match this condition.

Accepting spirv64-foo-bar, but not spirv64-nvidia-unknown would be somewhat odd. I think we either should check a fully-specified triple, or only check the parts that matter -- getArch() in this case, or, maybe, arch and vendor.

dcastagna added inline comments.Jan 24 2022, 1:51 PM
clang/lib/Driver/Driver.cpp
154–156

This part is the check for the hip offload triple and this patch did not change the logic for HIP (at least not intentionally), it should be the same as the logic specified in the current getHIPOffloadTargetTriple on ToT.
Happy to change it if you think it shuold be different though.

dcastagna updated this revision to Diff 402657.Jan 24 2022, 1:57 PM

Remove unknown-unknown check from HIP offload logic

dcastagna marked an inline comment as done.Jan 24 2022, 1:58 PM
dcastagna added inline comments.
clang/lib/Driver/Driver.cpp
154–156

Removed the vendor and os check to make it consistent with the cuda logic.

tra added a subscriber: linjamaki.Jan 24 2022, 2:01 PM
tra added inline comments.
clang/lib/Driver/Driver.cpp
154–156

@linjamaki, @yaxunl -- are you OK with ignoring the vendor/OS parts for spirv triples?

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.

yaxunl added a comment.EditedJan 25 2022, 6:22 AM

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.

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

One usage of vendor component in spirv triple is that it may be used to choose toolchain if there are multiple toolchains supporting spirv.

@mkuper What are intended use of OS and environment components of spirv triple?

tra added a comment.Jan 25 2022, 10:41 AM

SPIR-V target requires that the OS and the environment type is unknown (see TargetInfo::AllocateTarget and BaseSPIRTargetInfo).

The problem is that LLVM's triple parser will set UnknownVendor for *any* vendor it does not know about. As I've pointed in the previous comment positively checking for an unknown vendor leads to a somewhat odd situation, when a triple "vpirv64-whoknowswhat" will be accepted, but "spirv64-suse" will not, even though both are equally nonsensical as far as spirv is concerned.

If SPIRV needs a vendor-specific treatment, then it probably needs a specific vendor enum for that. UnknownVendor is ill-suited for that purpose.

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

Having a vendor component in the triple seems to be acceptable for the SPIR-V target.

tra added a comment.Jan 26 2022, 11:11 AM

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

My point is that unknown part of the triple is a catch-all for anything, including something invalid and should not be used for positive checks.
If we do not care about those parts of the triple ( accepting invalid triple would imply it), then we should not check those parts at all.
Otherwise it leads to a weird inconsistency -- invalid triple like spirv64-foo-baris accepted, but an equally nonsensical triple like spir64-suse-whateverwhich happens to use a known vendor or OS parts is not.

The bottom line is that if there's currently no known use of the vendor/OS/env parts of the triple, then we should not be checking them.
If we do want to accept specific triple, then appropriate enums should be used/added.

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

My point is that unknown part of the triple is a catch-all for anything, including something invalid and should not be used for positive checks.
If we do not care about those parts of the triple ( accepting invalid triple would imply it), then we should not check those parts at all.
Otherwise it leads to a weird inconsistency -- invalid triple like spirv64-foo-baris accepted, but an equally nonsensical triple like spir64-suse-whateverwhich happens to use a known vendor or OS parts is not.

The bottom line is that if there's currently no known use of the vendor/OS/env parts of the triple, then we should not be checking them.
If we do want to accept specific triple, then appropriate enums should be used/added.

I get your point. TT.getVendor() == llvm::Triple::UnknownVendor and TT.getOS() == llvm::Triple::UnknownOS checks the processed vendor/OS string instead of the original string, which could be misleading.

Since SPIRV backend requires OS and environment to be unknown. It seems we'd better check the original OS and environment string in the Triple by splitting the triple by - and taking the 3rd and 4th element (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Triple.cpp#L795).

@yaxunl Are you OK landing this change as it is, without the check for OS and environment in getHIPOffloadTargetTriple?
We can follow up with patch that adds checks for in OS and environment in Triple.cpp. Is that what you meant?

@yaxunl Are you OK landing this change as it is, without the check for OS and environment in getHIPOffloadTargetTriple?
We can follow up with patch that adds checks for in OS and environment in Triple.cpp. Is that what you meant?

LGTM. @tra @linjamaki What do you think?

tra accepted this revision.Jan 28 2022, 12:01 PM
This revision is now accepted and ready to land.Jan 28 2022, 12:01 PM
This revision was automatically updated to reflect the committed changes.

Pushed for Daniele:

To github.com:llvm/llvm-project.git
   99d2582164c4..6eb826567af0  main -> main