[OpenMP] Add flag for linking runtime bitcode library
ClosedPublic

Authored by gtbercea on Feb 12 2018, 8:57 AM.

Diff Detail

Repository
rL LLVM
There are a very large number of changes, so older changes are hidden. Show Older Changes
gtbercea marked an inline comment as done.Feb 15 2018, 2:39 PM

Looking more closely at the patch, this doesn't seem to look into the lib / lib64 next to the compiler. I'm not sure if LIBRARY_PATH is set for every installation, so I think we should add this one to catch the obvious case. This probably needs some attention for the tests because they'll find the just-built libraries...

lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

tools::addDirectoryList uses StringRef::find, I'm not sure if StringRef::split creates real copies of the string...

gtbercea added a comment.EditedMar 5 2018, 7:27 AM

Looking more closely at the patch, this doesn't seem to look into the lib / lib64 next to the compiler. I'm not sure if LIBRARY_PATH is set for every installation, so I think we should add this one to catch the obvious case. This probably needs some attention for the tests because they'll find the just-built libraries...

The contract with the user us that the .bc lib needs to be in LIBRARY_PATH, this is what we require today. Inlining of the runtime is not essential for correctness.

gtbercea added inline comments.Mar 5 2018, 7:44 AM
lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

What is your suggestion?

Looking more closely at the patch, this doesn't seem to look into the lib / lib64 next to the compiler. I'm not sure if LIBRARY_PATH is set for every installation, so I think we should add this one to catch the obvious case. This probably needs some attention for the tests because they'll find the just-built libraries...

The contract with the user us that the .bc lib needs to be in LIBRARY_PATH, this is what we require today. Inlining of the runtime is not essential for correctness.

And I don't think this is clever. The compiler should always try to automatically find the files it needs and looking in the lib directory next to the compiler installation isn't hard in that case and a guess that will meet 99% of deployments.

lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

IMO you should use whatever existing code does, in that case StringRef::find.

ABataev added inline comments.Mar 6 2018, 6:32 AM
lib/Driver/ToolChains/Cuda.cpp
535 ↗(On Diff #134295)
  1. char *->const char *
  2. ::getenv->llvm::Process::GetEnv
539 ↗(On Diff #134295)

Use llvm::SplitString instead

548 ↗(On Diff #134295)

const std::string &

gtbercea updated this revision to Diff 137203.Mar 6 2018, 8:26 AM

Address comments.

gtbercea marked 3 inline comments as done.Mar 6 2018, 8:27 AM
ABataev added inline comments.Mar 6 2018, 9:05 AM
lib/Driver/ToolChains/Cuda.cpp
591 ↗(On Diff #137203)

Maybe just LibraryPaths.emplace_back(DefaultLibPath);?

598 ↗(On Diff #137203)

Wow, never do such things! This is a pointer to non-null terminated string.
Instead

const char EnvPathSeparatorStr[] = {EnvPathSeparator, '\0'};

And use this array as a separator.

gtbercea updated this revision to Diff 137219.Mar 6 2018, 9:26 AM

Address comments.

ABataev added inline comments.Mar 6 2018, 9:41 AM
include/clang/Basic/DiagnosticDriverKinds.td
207–208 ↗(On Diff #137219)

Fix the message in the warning, it does not follow the logic of the patch

lib/Driver/ToolChains/Cuda.cpp
586 ↗(On Diff #137219)

Do you really need std::string here? Or StringRef is enough?

gtbercea updated this revision to Diff 137226.Mar 6 2018, 10:29 AM

Address comments.

gtbercea updated this revision to Diff 137230.Mar 6 2018, 10:39 AM

Fix test.

gtbercea updated this revision to Diff 137233.Mar 6 2018, 10:44 AM
  • Fix message and test.
gtbercea marked 3 inline comments as done.Mar 6 2018, 10:45 AM
gtbercea added inline comments.Mar 8 2018, 8:14 AM
lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

Is this comment still relevant in the light of the most recent changes?

Hahnfeld added inline comments.Mar 9 2018, 1:21 AM
lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

Probably not (although the code is now completely different from tools::addDirectoryList)

ABataev added inline comments.Mar 9 2018, 7:18 AM
lib/Driver/ToolChains/Cuda.cpp
592 ↗(On Diff #137233)

Do you still need .c_str() here?

test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

Create empty libomptarget-nvptx-sm_60.bc in Driver/lib directory and use it in the test rather create|delete it dynamically.

gtbercea marked an inline comment as done.Mar 9 2018, 7:19 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
536–542 ↗(On Diff #134295)

Gotcha, do let me know if you see any other issue with this version of the code. I will mark this one as done for now.

grokos added inline comments.Mar 9 2018, 7:35 AM
test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

I'm also in favour of this approach. On some systems /tmp is not accessible and the regression test fails.

gtbercea updated this revision to Diff 137754.Mar 9 2018, 7:49 AM

Change test.

Hahnfeld added inline comments.Mar 9 2018, 7:49 AM
test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

This test doesn't (and shouldn't!) use /tmp. The build directory and %T are always writable (if not, you have different issues on your system).

Btw you need to pay attention that the driver now finds files next to the compiler directory. You may want to make sure that the test always passes / doesn't fail for wrong reasons.

gtbercea added inline comments.Mar 9 2018, 7:50 AM
lib/Driver/ToolChains/Cuda.cpp
592 ↗(On Diff #137233)

Doesn't compile without it but we can get there using Args.MakeArgString() also.

test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

Just added this.

gtbercea marked 2 inline comments as done.Mar 9 2018, 7:52 AM
gtbercea updated this revision to Diff 137755.Mar 9 2018, 8:00 AM

Revert to c_str().

gtbercea added inline comments.Mar 9 2018, 8:03 AM
test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

@Hahnfeld I've used %S instead.

The only way in which the test can be a false positive is when the lib folder contains this .bc file. But there's no way to stop this from happening since we check DefaultLibPath first.

Hahnfeld added inline comments.Mar 9 2018, 8:40 AM
test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

(My comment was related to @grokos, the usage of %T and temporarily creating the bc lib. The current approach with %S/Inputs is much cleaner, but you need to create a subdirectory as everbody else did.)

Then you need to find a way to stop this. There already are some flags to change the sysroot etc., but I don't know if the influence what you use in this patch. In the worst case, you need to add a new flag to disable DefaultLibPath and use it in the tests. You can't propose to commit a test that is known to break (although I acknowledge that libomptarget-nvptx-sm_20.bc will probably never exist).

gtbercea updated this revision to Diff 137769.Mar 9 2018, 8:52 AM

Fix test.

test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

I created a lib folder where the empty .bc is present: %S/Inputs/lib

Good point. sm_20.bc cannot be created since libomptarget requires sm_30 at least which means that there can never be an sm_20 in the DefaultLibPath folder so the only way to find it is to follow LIBRARY_PATH. This resolves the issue.

Hahnfeld added inline comments.Mar 10 2018, 2:51 AM
test/Driver/openmp-offload-gpu.c
150 ↗(On Diff #137233)

Yes, and everybody else creates subdirectories with names that explain what they contain: CUDA, debian, mingw etc. You should pay more attention to follow a common style when already established.

Relying that sm_20 will never be there is maybe not the cleanest solution but should work here. I'm fine unless somebody else objects.

gtbercea marked an inline comment as done.Mar 12 2018, 7:08 AM
gtbercea updated this revision to Diff 137999.Mar 12 2018, 7:13 AM

Change name of folder.

gtbercea updated this revision to Diff 138000.Mar 12 2018, 7:18 AM

Rename folder. Fix test.

gtbercea updated this revision to Diff 138002.Mar 12 2018, 7:22 AM

Add input file.

ABataev added inline comments.Mar 13 2018, 9:21 AM
lib/Driver/ToolChains/Cuda.cpp
595–596 ↗(On Diff #138002)

Move the definition of LibPath out of if statement scope.

600 ↗(On Diff #138002)

auto->StringRef

607 ↗(On Diff #138002)

const std::string &->StringRef

gtbercea updated this revision to Diff 138242.Mar 13 2018, 11:56 AM

Address comments.

gtbercea marked 3 inline comments as done.Mar 13 2018, 11:57 AM
This revision was automatically updated to reflect the committed changes.
gtbercea reopened this revision.Mar 13 2018, 2:20 PM
This revision is now accepted and ready to land.Mar 13 2018, 2:20 PM
gtbercea updated this revision to Diff 138260.Mar 13 2018, 2:27 PM

Improve test robustness for the case when CUDA libdevice cannot be found.
Check that the warning is not emitted when the bc lib is found.

gtbercea updated this revision to Diff 138261.Mar 13 2018, 2:28 PM

Add bclib.

gtbercea updated this revision to Diff 138265.Mar 13 2018, 2:33 PM

Update patch manually.

gtbercea updated this revision to Diff 138266.Mar 13 2018, 2:42 PM
gtbercea updated this revision to Diff 138274.Mar 13 2018, 4:16 PM
This comment was removed by gtbercea.
gtbercea updated this revision to Diff 138275.Mar 13 2018, 4:18 PM
This revision was automatically updated to reflect the committed changes.