This is an archive of the discontinued LLVM Phabricator instance.

[HIPSPV][2/4] Add HIPSPV tool chain
ClosedPublic

Authored by linjamaki on Sep 28 2021, 2:50 AM.

Details

Summary

This patch adds a new tool chain, HIPSPVToolChain, for emitting HIP
device code as SPIR-V binary. The SPIR-V binary is emitted by using an
external tool, SPIRV-LLVM-Translator, temporarily. We intend to switch
the translator to the llc tool when the SPIR-V backend lands on LLVM
and proves to work well on HIP implementations which consume SPIR-V.

Before the SPIR-V emission the tool chain loads an optional external
pass plugin, either automatically from a HIP installation or from a
path pointed by --hipspv-pass-plugin, and runs passes that are meant
to expand/lower HIP features that do not have direct counterpart in
SPIR-V (e.g. dynamic shared memory).

Code emission for SPIR-V will be enabled and HIPSPVToolChain tests
will be added in the follow up patch part 3.

Other changes: New option ‘-nohipwrapperinc’ is added to exclude HIP
include wrappers. The reason for the addition is that they cause
compile errors when compiling HIP sources for the host side for HIPCL
and HIPLZ implementations. New option is added to avoid this issue.

Diff Detail

Event Timeline

linjamaki created this revision.Sep 28 2021, 2:50 AM
linjamaki updated this revision to Diff 375526.Sep 28 2021, 4:36 AM

Style fixes.

linjamaki published this revision for review.Sep 28 2021, 5:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 5:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
linjamaki added subscribers: bader, Anastasia.
tra added a subscriber: tra.Sep 29 2021, 10:07 AM
tra added inline comments.
clang/include/clang/Driver/Options.td
3750

Is the idea to still add relevant include paths to the wrappers and SDK headers, but not -include the wrapper?

If that's the case, it should probably be generalized into -nogpuwrapperinc and apply to both CUDA and HIP.

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.

Another question I have is whether --hipspv-pass-plugin would be needed when we switch to SPIR-V backend or is this something that would be integrated fully upstream eventually. Then we might need to think more of the suitable transition path.

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.

Another question I have is whether --hipspv-pass-plugin would be needed when we switch to SPIR-V backend or is this something that would be integrated fully upstream eventually. Then we might need to think more of the suitable transition path.

It’s too early to say if we can integrate --hipspv-pass-plugin fully in the future. It would be the preferred outcome. We are not finished with the HIP expanders we need for supporting various HIP features so we don’t know for certain what will be the outcome for the integration. It’s possible that some of the solutions are going to be tightly coupled with runtimes (HIPLZ, HIPCL) and others that may not generalize for different SPIR-V execution environments.

clang/include/clang/Driver/Options.td
3750

Is the idea to still add relevant include paths to the wrappers and SDK headers, but not -include the wrapper?

Include paths are meant to be excluded too. I’ll fix the option description.

If that's the case, it should probably be generalized into -nogpuwrapperinc and apply to both CUDA and HIP.

I don’t see an immediate need to generalize the option as I don’t think there will be a need for it in the CUDA path. The option could be generalized later if the need comes (add generalized option, set -nohipwrapperinc to be alias to it).

linjamaki updated this revision to Diff 376822.Oct 4 2021, 1:52 AM

Update option description.

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.

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.

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 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 believe this is indeed the most common functionality at this point, so if we could factor it out already now it would make sense. However, towards the common tooling we would probably need to create a special Tool/ToolChain for SPIR-V which could be derived from or used for the languages/toochains that target SPIR-V generation.

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

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

Update for changes in D112404.

LGTM. I will leave to @tra about -nohipwrapperinc

tra accepted this revision.Nov 22 2021, 1:01 PM

LGTM in general, modulo push_back/append nits.

clang/include/clang/Driver/Options.td
3750

Fair enough. Indeed, without the wrappers, we will not be able to parse CUDA SDK headers.

clang/lib/Driver/ToolChains/HIPSPV.cpp
146

Nit: combine with -fcuda-is-device into append({})

152–153

Nit: Combine all unconditional push_back() calls into append();

165–166

Nit: combine into single append.

210–211

-> append({}).

This revision is now accepted and ready to land.Nov 22 2021, 1:01 PM
linjamaki updated this revision to Diff 389112.Nov 23 2021, 1:16 AM

Combine options with append().

linjamaki updated this revision to Diff 389118.Nov 23 2021, 1:28 AM

Retry push changes.

linjamaki marked 4 inline comments as done.Nov 23 2021, 1:30 AM

Thanks for the review.

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?

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?

HIPSPV primarily relies on the system’s PATH for locating the translator.

Additionally, the translator is sought in the same directory where the Clang driver is installed in. This is rather a side-effect than a feature. The directory is added to Clang’s internal search paths for looking up the clang-offload-bundler tool required by the HIPSPV tool chain.

  • Is there any diagnostic provided if the translator not installed/found?

Clang displays the following standard error if the translator is not found:

error: unable to execute command: Executable "llvm-spirv" doesn't exist!
  • 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?

A version mismatch may result in an error in the translator. This is a known issue and a reason we want to switch to the SPIR-V backend when it lands on the LLVM - the translator is meant to be used temporarily by the HIPSPV. The synchronization could be improved by having the Clang to seek a SPIR-V Translator binary that is named with the LLVM version it has been built against - e.g. “llvm-spirv-14” for the next LLVM release.

AFAIK, Clang’s infrastructure does not support early diagnosis on external tools. The diagnosis during Clang’s build and installation probably won’t matter much if the Clang is distributed as binary to other systems - the destination system might not have the required translator version.

linjamaki updated this revision to Diff 389702.Nov 25 2021, 2:26 AM

Disable debug info generation for device code.

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?

HIPSPV primarily relies on the system’s PATH for locating the translator.

Additionally, the translator is sought in the same directory where the Clang driver is installed in. This is rather a side-effect than a feature. The directory is added to Clang’s internal search paths for looking up the clang-offload-bundler tool required by the HIPSPV tool chain.

  • Is there any diagnostic provided if the translator not installed/found?

Clang displays the following standard error if the translator is not found:

error: unable to execute command: Executable "llvm-spirv" doesn't exist!
  • 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?

A version mismatch may result in an error in the translator. This is a known issue and a reason we want to switch to the SPIR-V backend when it lands on the LLVM - the translator is meant to be used temporarily by the HIPSPV. The synchronization could be improved by having the Clang to seek a SPIR-V Translator binary that is named with the LLVM version it has been built against - e.g. “llvm-spirv-14” for the next LLVM release.

AFAIK, Clang’s infrastructure does not support early diagnosis on external tools. The diagnosis during Clang’s build and installation probably won’t matter much if the Clang is distributed as binary to other systems - the destination system might not have the required translator version.

Alternatively, we could see if translator could emit a diagnostic by looking at the clang version emitted in metadata... I presume this might not be too hard to implement...

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

This revision was landed with ongoing or failed builds.Dec 14 2021, 10:23 AM
Closed by commit rG4e94cba5b4e4: [HIPSPV][2/4] Add HIPSPV tool chain (authored by linjamaki, committed by tra). · Explain Why
This revision was automatically updated to reflect the committed changes.