This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] Fix libdevice selection.
ClosedPublic

Authored by tra on Aug 1 2016, 2:42 PM.

Details

Summary

This makes clang's libdevice selection match that of NVCC as described in
http://docs.nvidia.com/cuda/libdevice-users-guide/basic-usage.html#version-selection

If required libdevice variant is not found, driver now fails with an error.

Diff Detail

Repository
rL LLVM

Event Timeline

tra updated this revision to Diff 66385.Aug 1 2016, 2:42 PM
tra retitled this revision from to [CUDA] Fix libdevice selection..
tra updated this object.
tra added a reviewer: jlebar.
tra added a subscriber: cfe-commits.
jlebar added inline comments.Aug 1 2016, 2:48 PM
lib/Driver/ToolChains.cpp
4773 ↗(On Diff #66385)

I *think* march is never empty here. We should add it when we create the CudaActions.

Same below.

If I'm right or if I'm wrong, let's make sure there are tests. :)

test/Driver/cuda-detect.cu
66 ↗(On Diff #66385)

Should we test the new sm_XX --> libdevice file mappings added?

tra updated this revision to Diff 66392.Aug 1 2016, 3:21 PM

Added tests for libdevice mapping correctness.

lib/Driver/ToolChains.cpp
4773 ↗(On Diff #66385)

It's guaranteed to be non-empty here only when we're constructing device-side pipeline which *is* normally the case.

Alas, it's possible for user to specify a nonsensical set of arguments that would trigger this assertion:

For instance: clang --cuda-host-only -target nvptx-nvidia-cuda -x cuda used by test/Preprocessor/cuda-types.cu.

Because target is CUDA, we do pick CudaToolchain, but because it's a host compilation, we never get to add -march=sm_XX (TC->TranslateArgs is called with BoundArch=nullptr).

Dafaulting to somewhat safe sm_20 on user error seems better than crashing compiler with an assertion.

test/Driver/cuda-detect.cu
66 ↗(On Diff #66385)

OK.

jlebar added inline comments.Aug 1 2016, 3:24 PM
lib/Driver/ToolChains.cpp
4773 ↗(On Diff #66392)

Hm. I totally agree that we shouldn't crash (especially because assertions are disabled in production builds!). But I'm not sure that defaulting to sm_20 every time we read this option is right. "This option must always be set" seems like an easier invariant to enforce than "whenever you read this option, you must use a default of sm_20, unless you're really sure that it's always set. And if you think you're really sure, you're probably wrong." :)

tra updated this revision to Diff 66579.Aug 2 2016, 4:01 PM

Now that D23042 / r277537 makes sure that CudaToolchain is only used on device side, we can
remove defaults for -march and restore asserts() ensuring that -march is added by the driver.

tra marked an inline comment as done.Aug 2 2016, 4:02 PM
jlebar accepted this revision.Aug 2 2016, 4:05 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Aug 2 2016, 4:05 PM
This revision was automatically updated to reflect the committed changes.