This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add flag for linking runtime bitcode library
ClosedPublic

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

Diff Detail

Repository
rC Clang

Event Timeline

gtbercea created this revision.Feb 12 2018, 8:57 AM
grokos added inline comments.Feb 12 2018, 9:12 AM
lib/Driver/ToolChains/Cuda.cpp
611

Should we be more specific when it comes to the name of the missing bc file and include the sm version? E.g. we may have libomptarget-nvptx-sm35.bc in LIBRARY_PATH but the driver needs libomptarget-nvptx-sm60.bc. If the user gets a general missing libomptarget-nvptx.bc message, it may not be clear what the problem is.

gtbercea updated this revision to Diff 133882.Feb 12 2018, 9:24 AM

Fix warning message.

gtbercea marked an inline comment as done.Feb 12 2018, 9:24 AM
grokos accepted this revision.Feb 12 2018, 11:43 AM

I don't have any other remarks, looks good.

This revision is now accepted and ready to land.Feb 12 2018, 11:43 AM
gtbercea updated this revision to Diff 133919.Feb 12 2018, 12:52 PM

Add regression tests.

Hahnfeld added inline comments.Feb 13 2018, 1:26 AM
test/Driver/openmp-offload-gpu.c
150

This should not be in /tmp but probably %T.

151

You may want to add env which should make this check portable because lit on Windows does the right thing then (I don't know if this test is run on Windows, it probably is)

gtbercea updated this revision to Diff 134235.Feb 14 2018, 8:19 AM

Move unix specific test to new file.

Hahnfeld added inline comments.Feb 14 2018, 8:21 AM
test/Driver/unix-openmp-offload-gpu.c
15 ↗(On Diff #134235)

I don't see how that solves the problem of using /tmp?!?

gtbercea marked an inline comment as done.Feb 14 2018, 8:21 AM
gtbercea added inline comments.
test/Driver/openmp-offload-gpu.c
150

I don't think this would have worked since I need to create a file with a specific name in a folder somewhere and the separator is OS specific. I moved the test to a new file where I limit OS to linux.

gtbercea updated this revision to Diff 134238.Feb 14 2018, 8:23 AM

Fix tmp folder name.

gtbercea marked an inline comment as done.Feb 14 2018, 8:23 AM
Hahnfeld added inline comments.Feb 14 2018, 8:30 AM
test/Driver/openmp-offload-gpu.c
150

I'm pretty sure lit takes care of this, there are a lot of other tests that do so.

test/Driver/unix-openmp-offload-gpu.c
15 ↗(On Diff #134235)

(Interesting that this works with %t, the documentation mentions %T for a directory. But as other test cases do the same...)

gtbercea added inline comments.Feb 14 2018, 8:37 AM
test/Driver/unix-openmp-offload-gpu.c
15 ↗(On Diff #134235)

%T works too I just tried it. Any preference as to which one to use?

Hahnfeld added inline comments.Feb 14 2018, 10:09 AM
test/Driver/unix-openmp-offload-gpu.c
15 ↗(On Diff #134235)

No not really. The Clang tests aren't consistent so I don't think it matters...

gtbercea marked an inline comment as done.Feb 14 2018, 11:32 AM

I'm still not sure we can't run this test on Windows. I think lots of other tests use touch, even some specific to Windows...

I'm still not sure we can't run this test on Windows. I think lots of other tests use touch, even some specific to Windows...

Let me know what you'd like me to do. I can add the test back. I do see other tests not worrying about this so maybe I can do the same here...

I'm still not sure we can't run this test on Windows. I think lots of other tests use touch, even some specific to Windows...

Let me know what you'd like me to do. I can add the test back. I do see other tests not worrying about this so maybe I can do the same here...

To make it clear, I think doing all checks in openmp-offload-gpu.c increases coverage and will work as other tests show.

gtbercea updated this revision to Diff 134295.Feb 14 2018, 12:27 PM
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
588–594

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
588–594

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
588–594

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
587
  1. char *->const char *
  2. ::getenv->llvm::Process::GetEnv
591

Use llvm::SplitString instead

600

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

Maybe just LibraryPaths.emplace_back(DefaultLibPath);?

598

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

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

lib/Driver/ToolChains/Cuda.cpp
586

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
588–594

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
588–594

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

Do you still need .c_str() here?

test/Driver/openmp-offload-gpu.c
150

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
588–594

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

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

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

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

test/Driver/openmp-offload-gpu.c
150

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

@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

(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

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

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

Move the definition of LibPath out of if statement scope.

600

auto->StringRef

607

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.

Harbormaster completed remote builds in B16041: Diff 138262.
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.