-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[HIP][SPIRV] Enable the SPIRV backend instead of the translator through an experimental flag. #162282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[HIP][SPIRV] Enable the SPIRV backend instead of the translator through an experimental flag. #162282
Changes from all commits
e89ce89
8d56d95
d69850d
13d77d9
08b9512
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,7 @@ TYPE("c++-module-cpp-output", PP_CXXModule, INVALID, "iim", phases | |
TYPE("ada", Ada, INVALID, nullptr, phases::Compile, phases::Backend, phases::Assemble, phases::Link) | ||
TYPE("assembler", PP_Asm, INVALID, "s", phases::Assemble, phases::Link) | ||
TYPE("assembler-with-cpp", Asm, PP_Asm, "S", phases::Preprocess, phases::Assemble, phases::Link) | ||
TYPE("spv", SPV, INVALID, "spv", phases::Backend) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SPIR-V in this context is just the assembler face as far as I'm aware, I think there's some magic setting that splits it if you need to. That's what CUDA does w/ its PTX target. I'd imagine this would imply clang consuming SPIR-V to give you back an ELF or LLVM-IR. |
||
|
||
// Note: The `phases::Preprocess` phase is added to ".i" (i.e. Fortran | ||
// pre-processed) files. The reason is that the pre-processor "phase" has to be | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3672,29 +3672,45 @@ class OffloadingActionBuilder final { | |
// compiler phases, including backend and assemble phases. | ||
ActionList AL; | ||
Action *BackendAction = nullptr; | ||
bool AssembleAndLink = true; | ||
if (ToolChains.front()->getTriple().isSPIRV() || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SPIR-V here should be similar to what CUDA does, I would just expect to skip this step entirely. I'm fairly certain the normal pipeline works for SPIR-V since the standalone clang target does what I expect. The linker is probably the only bit you'd skip, the handling there is a little weird since CUDA overloads it to mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhuber6 thanks for the feedback! I'm not sure if I follow what's the problem or suggestion. What step(s) and for what scenario(s) can be skipped? If so, where that should/would be handled? A backend job was always created independently of SPIRV prior to the PR. Now it has been updated so it does using the SPIRV backend directly if needed. The assembler and linker steps are now skipped for SPIRV when using the backend. When using the translator, we still have to maintain the previous phases. |
||
(ToolChains.front()->getTriple().isAMDGCN() && | ||
GpuArchList[I] == StringRef("amdgcnspirv"))) { | ||
// Emit LLVM bitcode for SPIR-V targets. SPIR-V device tool chain | ||
// (HIPSPVToolChain or HIPAMDToolChain) runs post-link LLVM IR | ||
// passes. | ||
types::ID Output = Args.hasArg(options::OPT_S) | ||
|
||
bool UseSPIRVBackend = | ||
Args.hasFlag(options::OPT_use_experimental_spirv_backend, | ||
options::OPT_no_use_experimental_spirv_backend, | ||
/*Default=*/false); | ||
|
||
types::ID Output = UseSPIRVBackend ? types::TY_SPV | ||
: Args.hasArg(options::OPT_S) | ||
? types::TY_LLVM_IR | ||
: types::TY_LLVM_BC; | ||
|
||
BackendAction = | ||
C.MakeAction<BackendJobAction>(CudaDeviceActions[I], Output); | ||
|
||
if (UseSPIRVBackend) { | ||
AssembleAndLink = false; | ||
} | ||
|
||
} else | ||
BackendAction = C.getDriver().ConstructPhaseAction( | ||
C, Args, phases::Backend, CudaDeviceActions[I], | ||
AssociatedOffloadKind); | ||
auto AssembleAction = C.getDriver().ConstructPhaseAction( | ||
C, Args, phases::Assemble, BackendAction, | ||
AssociatedOffloadKind); | ||
AL.push_back(AssembleAction); | ||
// Create a link action to link device IR with device library | ||
// and generate ISA. | ||
CudaDeviceActions[I] = | ||
C.MakeAction<LinkJobAction>(AL, types::TY_Image); | ||
|
||
if (AssembleAndLink) { | ||
auto AssembleAction = C.getDriver().ConstructPhaseAction( | ||
C, Args, phases::Assemble, BackendAction, | ||
AssociatedOffloadKind); | ||
AL.push_back(AssembleAction); | ||
// Create a link action to link device IR with device library | ||
// and generate ISA. | ||
CudaDeviceActions[I] = | ||
C.MakeAction<LinkJobAction>(AL, types::TY_Image); | ||
} else { | ||
CudaDeviceActions[I] = BackendAction; | ||
} | ||
} | ||
|
||
// OffloadingActionBuilder propagates device arch until an offload | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// RUN: %clang -x hip %s --cuda-device-only --offload-arch=amdgcnspirv -use-experimental-spirv-backend -nogpuinc -nogpulib -ccc-print-bindings 2>&1 | FileCheck %s | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs a test for the new driver, because if all goes well I'm going to completely delete the old offload driver next release. |
||
|
||
// CHECK: # "spirv64-amd-amdhsa" - "clang", inputs: ["{{.*}}.c"], output: "[[SPV_FILE:.*.spv]]" | ||
// CHECK: # "spirv64-amd-amdhsa" - "AMDGCN::Linker", inputs: ["[[SPV_FILE]]"], output: "{{.*.hipfb}}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// This test case validates the behavior of -use-experimental-spirv-backend | ||
|
||
// Test that -use-experimental-spirv-backend calls clang -cc1 with the SPIRV triple. | ||
mgcarrasco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// RUN: %clang -x hip %s --cuda-device-only --offload-arch=amdgcnspirv -use-experimental-spirv-backend -nogpuinc -nogpulib -### 2>&1 | FileCheck %s --check-prefix=CHECK-SPIRV-BACKEND | ||
// CHECK-SPIRV-BACKEND: "{{.*}}clang{{.*}}" "-cc1" "{{.*-triple}}" "{{spirv64-amd-amdhsa}}" | ||
|
||
// Test that -no-use-experimental-spirv-backend calls the SPIRV translator | ||
// RUN: %clang -x hip %s --cuda-device-only --offload-arch=amdgcnspirv -no-use-experimental-spirv-backend -nogpuinc -nogpulib -### 2>&1 | FileCheck %s --check-prefix=CHECK-SPIRV-TRANSLATOR | ||
// CHECK-SPIRV-TRANSLATOR: "{{.*llvm-spirv.*}}" "{{--spirv-max-version=[0-9]+\.[0-9]}}" | ||
|
||
// Test that by default we use the translator | ||
// RUN: %clang -x hip %s --cuda-device-only --offload-arch=amdgcnspirv -nogpuinc -nogpulib -### 2>&1 | FileCheck %s --check-prefix=CHECK-SPIRV-TRANSLATOR |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// RUN: %clang -x hip %s --cuda-device-only --offload-arch=amdgcnspirv -use-experimental-spirv-backend -nogpuinc -nogpulib -ccc-print-phases 2>&1 | FileCheck %s | ||
|
||
// CHECK: [[P0:[0-9]+]]: input, "{{.*}}.c", hip, (device-hip, amdgcnspirv) | ||
// CHECK: [[P1:[0-9]+]]: preprocessor, {[[P0]]}, hip-cpp-output, (device-hip, amdgcnspirv) | ||
// CHECK: [[P2:[0-9]+]]: compiler, {[[P1]]}, ir, (device-hip, amdgcnspirv) | ||
// CHECK: [[P3:[0-9]+]]: backend, {[[P2]]}, spv, (device-hip, amdgcnspirv) | ||
// CHECK: [[P4:[0-9]+]]: offload, "device-hip (spirv64-amd-amdhsa:amdgcnspirv)" {[[P3]]}, spv | ||
// CHECK: [[P5:[0-9]+]]: linker, {[[P4]]}, hip-fatbin, (device-hip, ) | ||
// CHECK: [[P6:[0-9]+]]: offload, "device-hip (spirv64-amd-amdhsa:)" {[[P5]]}, hip-fatbin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a
defm
helper forno
variants right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty. If we don't require a member variable, then
defm
does not seem to be the right option (see howgpu_bundle_output
andno_gpu_bundle_output
are defined).