Page MenuHomePhabricator

[CUDA][HIP] Fix bound arch for offload action for fat binary
ClosedPublic

Authored by yaxunl on Tue, Sep 29, 6:56 PM.

Details

Summary

Currently CUDA/HIP toolchain uses "unknown" as bound arch
for offload action for fat binary. This causes -mcpu or -march
with "unknown" added in HIPToolChain::TranslateArgs or
CUDAToolChain::TranslateArgs.

This causes issue for https://reviews.llvm.org/D88377 since
HIP toolchain needs to check -mcpu in HIPToolChain::TranslateArgs.

The bound arch of offload action for fat binary is not really
used, therefore set it to nullptr to make things simpler.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Tue, Sep 29, 6:56 PM
yaxunl created this revision.
tra added a comment.Wed, Sep 30, 11:03 AM

Currently CUDA/HIP toolchain uses "unknown" as bound arch
for offload action for fat binary. This causes -mcpu or -march
with "unknown" added in HIPToolChain::TranslateArgs or
CUDAToolChain::TranslateArgs.

It would appear that the problem is actually where we check TargetID -- we should've ignored CudaArch::UNKNOWN there.
Not setting the arch here avoids triggering the bug but it does not fix it.
Considering that CudaArch::UNKNOWN is used here to indicate that the arch is unused, perhaps we need an enum for that to distinguish it from unknown/unset.

In D88524#2304173, @tra wrote:

Currently CUDA/HIP toolchain uses "unknown" as bound arch
for offload action for fat binary. This causes -mcpu or -march
with "unknown" added in HIPToolChain::TranslateArgs or
CUDAToolChain::TranslateArgs.

It would appear that the problem is actually where we check TargetID -- we should've ignored CudaArch::UNKNOWN there.
Not setting the arch here avoids triggering the bug but it does not fix it.
Considering that CudaArch::UNKNOWN is used here to indicate that the arch is unused, perhaps we need an enum for that to distinguish it from unknown/unset.

This translates to "unknown" in string form, which feels arbitrary. What if a target has a valid cpu name which "unknown"? Isn't a nullptr (empty string in string form) a more generic format to represent an invalid target?

Is it OK to make the change HIP specific? i.e. let HIP toolchain use empty string for invalid target whereas CUDA toolchain keep using "unknown".

tra added a comment.Wed, Sep 30, 11:31 AM

This translates to "unknown" in string form, which feels arbitrary. What if a target has a valid cpu name which "unknown"? Isn't a nullptr (empty string in string form) a more generic format to represent an invalid target?

It was the nullptr that got me digging. I was not sure what it would mean and do. Generally speaking it's a 'magic' value with the meaning that's not at all clear, even if it does what you want. I'd rather have a constant or enum with a clear meaning.

Is it OK to make the change HIP specific? i.e. let HIP toolchain use empty string for invalid target whereas CUDA toolchain keep using "unknown".

Putting magic value behind if(HIP) does not make it any better.

TargetID(nullptr) is the same as TargetID("") because StringRef(nullptr) is the same as an empty string. I'd add a CudaArch::UNUSED and modify CudaVersionToString to return "". That should make HIPToolChain::TranslateArgs()skip it as it would for nullptr.

yaxunl updated this revision to Diff 295572.Thu, Oct 1, 7:45 AM

add CudaArch::UNUSED as suggested by Artem.

tra accepted this revision.Thu, Oct 1, 10:34 AM
tra added inline comments.
clang/lib/Basic/Cuda.cpp
96

You could add a {CudaArch::UNUSED, "", ""} to the table above instead of explicit checks here.

This revision is now accepted and ready to land.Thu, Oct 1, 10:34 AM
yaxunl marked an inline comment as done.Thu, Oct 1, 1:22 PM
yaxunl added inline comments.
clang/lib/Basic/Cuda.cpp
96

will do when committing

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 2, 4:14 PM