Page MenuHomePhabricator

[OpenMP] Enable on device linking with NVLINK to ignore dynamic libraries
Needs ReviewPublic

Authored by gtbercea on Mar 6 2019, 8:51 AM.

Details

Summary

NVLINK does not support dynamic linking so passing dynamic libraries to NVLINK should be avoided.

This patch fixes a bug whereby an explicit passing of the dynamic library via it's full path will result in it being treated like an input.

Diff Detail

Repository
rC Clang

Event Timeline

gtbercea created this revision.Mar 6 2019, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 8:51 AM
ABataev added inline comments.Mar 6 2019, 9:43 AM
lib/Driver/ToolChains/Cuda.cpp
707

I would suggest to use types::lookupTypeForExtension function and compare the result with the types::TY_Object.

gtbercea marked an inline comment as done.Mar 6 2019, 10:18 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
707

Isn't that too broad? I only want .so files.

ABataev added inline comments.Mar 6 2019, 10:20 AM
lib/Driver/ToolChains/Cuda.cpp
707

I don't think nvlink supports anything else except for objects. Also, in future, we could extend it if it is required.

gtbercea marked an inline comment as done.Mar 6 2019, 10:23 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
707

But won't this exclude .o files too? I don't want to ignore those.

ABataev added inline comments.Mar 6 2019, 10:28 AM
lib/Driver/ToolChains/Cuda.cpp
707

This function returns TY_Object for .o and .obj extensions (on Windows)

gtbercea marked an inline comment as done.Mar 6 2019, 10:47 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
707

Oh now I think I understand what you mean. You mean ignore everything that's not a TY_Object? That doesn't work here because I am looking at the original input file name.

ABataev added inline comments.Mar 6 2019, 10:48 AM
lib/Driver/ToolChains/Cuda.cpp
707

Yes, but you can use types::lookupTypeForExtension function to classify the original input file name.

For now this patch doesn't make sense until the main linking patch lands but it's a case we need to keep in mind.