This is an archive of the discontinued LLVM Phabricator instance.

Add -L/path/to/cuda/lib if any of our inputs are CUDA files.
AbandonedPublic

Authored by jlebar on Dec 16 2015, 4:01 PM.

Details

Reviewers
tra
echristo

Diff Detail

Event Timeline

jlebar updated this revision to Diff 43080.Dec 16 2015, 4:01 PM
jlebar retitled this revision from to Add -L/path/to/cuda/lib if any of our inputs are CUDA files..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added subscribers: tra, jhen.
tra added inline comments.Dec 16 2015, 4:18 PM
lib/Driver/ToolChains.cpp
4125

We may need a new option as -nocudalib is currently used to disable linking with libdevice bitcode library.

lib/Driver/Tools.cpp
229

Perhaps you want to use isa<CudaDeviceAction>() here.

tra edited edge metadata.Dec 16 2015, 4:20 PM

You also need a test case for the changes.

tra added inline comments.Dec 16 2015, 4:21 PM
lib/Driver/Tools.cpp
277–279

Style nit -- no need for curly braces.

jlebar updated this revision to Diff 44056.Jan 5 2016, 1:50 PM
jlebar marked 2 inline comments as done.
jlebar edited edge metadata.

Address tra's review comments.

jlebar added a comment.Jan 5 2016, 1:50 PM

I'm sorry I sat on this for so long; I failed at email somehow, and only came back to ping this patch. :)

lib/Driver/ToolChains.cpp
4125

Hm...I don't want to over-complicated the UI. Is there prior art for what we're doing here that we could crib off?

tra added inline comments.Jan 5 2016, 5:03 PM
lib/Driver/ToolChains.cpp
4125

I'd rename -nocudalib to -nocudalibdevice (or -nocudabclib) to better reflect what it currently does -- disables linking with CUDA's libdevice bitcode.

Then you could use -nocudalib to control automatic addition of CUDA library-related options which would be closer to what '-nostdlib' does.

While you're at it, you may consider adding linker flags to link with static libcudart. nvcc always adds "-lcudart_static -lrt -lpthread -ldl" at the end. If user explicitly requests linking with dynamic version of libcudart, then it gets linked first, and linker then ignores all the symbols from static lib.

jlebar added inline comments.Jan 6 2016, 10:45 AM
lib/Driver/ToolChains.cpp
4125

Sounds like a plan. I'll send separate patches for each of these.

jlebar updated this revision to Diff 44145.Jan 6 2016, 11:57 AM

Now also pass -lcudart_static -lcuda -ldl -lrt -pthread for CUDA compiles.

Depends on D15933.

jlebar marked an inline comment as done.Jan 6 2016, 12:57 PM

Done, please have another look.

tra added inline comments.Jan 6 2016, 1:49 PM
include/clang/Driver/Options.td
1636

include -> link with

Perhaps less implementation details would be more appropriate for a help string.
"Don't link with default CUDA runtime library"?

lib/Driver/ToolChains.cpp
4129–4137

Does this kick in before user's arguments have been added or after?
If user args have not been added yet, then you'll never find them and don't need this check at all.

Skipping -lFOO will also have problems if any of the libraries are static and one of preceding libraries needs extra symbols that were not linked in by this point yet.

Link with required libraries unconditionally. Make sure to add these libraries after user arguments have been added.

4139

-lcuda is not needed. As far as I can tell, cudart_static will open and use it via dlopen.

It would be needed only if user uses any of API calls from libcuda directly, but then it would be up to user to add -lcuda.

4142

-lpthread

jlebar updated this revision to Diff 44156.Jan 6 2016, 2:03 PM
jlebar marked 4 inline comments as done.

Address tra's review comments.

jlebar added a comment.Jan 6 2016, 2:11 PM

Updated code; please have a look.

include/clang/Driver/Options.td
1636

include -> link with

Done.

Perhaps less implementation details would be more appropriate for a help string.

I'm not sure about that? Maybe if we were just including -lcudart_static, but given that we're pulling in other libraries and adding a library search path, I feel like we should tell users what we're doing? I mean, we should document it somewhere, and here's where people are actually going to find it...

lib/Driver/ToolChains.cpp
4129–4137

Does this kick in before user's arguments have been added or after?

After; I wrote tests. :)

Link with required libraries unconditionally.

Done.

4142

Oh, I see, -pthread is a compile-time flag, not a link-time flag. Thanks.

jlebar updated this revision to Diff 44160.Jan 6 2016, 2:21 PM

Adding back testcase that ensures that our flags are added after user flags.

jlebar added inline comments.Jan 6 2016, 2:26 PM
lib/Driver/ToolChains.cpp
4129–4137

Although then I deleted the relevant tests... I just added new ones. :)

tra accepted this revision.Jan 6 2016, 2:35 PM
tra edited edge metadata.

Minor nit, but looks good otherwise.

include/clang/Driver/Options.td
1636

Implementation details may be different on different platforms. I.e. you may need to link with different set of libraries if/when we add missing bits for windows and your help string will make no sense there.

Specifics should be in the documentation, IMO.

lib/Driver/ToolChains.cpp
4130

{}, again.
:-)

This revision is now accepted and ready to land.Jan 6 2016, 2:35 PM
jlebar updated this revision to Diff 44170.Jan 6 2016, 3:07 PM
jlebar marked 2 inline comments as done.
jlebar edited edge metadata.

Address tra's review comments.

include/clang/Driver/Options.td
1636

OK, that's a good reason not to do it in the command-line help.

jlebar updated this revision to Diff 44178.Jan 6 2016, 3:56 PM

Split out the bits adding an arg to AddLinkerInputs into a separate patch, D15939.

tra added inline comments.Jan 6 2016, 4:18 PM
lib/Driver/Tools.cpp
276

It just struck me that this patch may not be as useful as we'd like it to be.

For typical compilation source-to-object and object-to-executable are separate phases.
We need these flags during linking phase, but we will rarely see any CUDA inputs passed to clang. Most of the time we'll be dealing with .o files and will have no way to tell whether those were compiled from CUDA sources. :-(

jlebar abandoned this revision.Jan 6 2016, 4:24 PM

It just struck me that this patch may not be as useful as we'd like it to be.

Oh. Yes.

I guess I'll drop this and D15939. :(