[OpenMP] Add flag for linking runtime bitcode library

[gtbercea]

Summary

This patch adds an additional flag to the OpenMP device offloading toolchain to link in the runtime library bitcode.

[grokos]

lib/Driver/ToolChains/Cuda.cpp

559 ↗

(On Diff #133877)

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]

Fix warning message.

[grokos]

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

[gtbercea]

Add regression tests.

[Hahnfeld]

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]

Move unix specific test to new file.

[Hahnfeld]

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

15 ↗

(On Diff #134235)

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

[gtbercea]

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]

Fix tmp folder name.

[Hahnfeld]

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]

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]

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]

Use %T.

[Hahnfeld]

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...

[gtbercea]

In D43197#1007918, @Hahnfeld wrote:

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...

[Hahnfeld]

In D43197#1007963, @gtbercea wrote:

In D43197#1007918, @Hahnfeld wrote:

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]

Revert.

[gtbercea]

Fix test.

[Hahnfeld]

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]

In D43197#1011256, @Hahnfeld wrote:

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]

lib/Driver/ToolChains/Cuda.cpp

536–542 ↗

(On Diff #134295)

What is your suggestion?

[Hahnfeld]

In D43197#1027139, @gtbercea wrote:

In D43197#1011256, @Hahnfeld wrote:

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]

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]

Address comments.

[ABataev]

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]

Address comments.

[ABataev]

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]

Address comments.

[gtbercea]

Fix test.

[gtbercea]

• Fix message and test.

[gtbercea]

lib/Driver/ToolChains/Cuda.cpp

536–542 ↗

(On Diff #134295)

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

[Hahnfeld]

lib/Driver/ToolChains/Cuda.cpp

536–542 ↗

(On Diff #134295)

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

[ABataev]

lib/Driver/ToolChains/Cuda.cpp

592 ↗

(On Diff #137233)

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]

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]

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]

Change test.

[Hahnfeld]

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]

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

Just added this.

[gtbercea]

Revert to c_str().

[gtbercea]

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]

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]

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]

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]

Change name of folder.

[gtbercea]

Rename folder. Fix test.

[gtbercea]

Fixes.

[gtbercea]

Add input file.

[ABataev]

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]

Address comments.

[ABataev]

LG

[gtbercea]

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]

Add bclib.

[gtbercea]

Test.

[gtbercea]

Update patch manually.

[gtbercea]

This comment was removed by gtbercea.

test/Driver/openmp-offload-gpu.c

 // RUN:   | FileCheck -check-prefix=CHK-NOLIBDEVICE %s
 
 // CHK-NOLIBDEVICE-NOT: error:{{.*}}sm_60
 
 /// ###########################################################################
 
 /// Check that the runtime bitcode library is part of the compile line. Create a bogus
 /// bitcode library and add it to the LIBRARY_PATH.
 // RUN:   env LIBRARY_PATH=%S/Inputs/libomptarget %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
-// RUN:   -Xopenmp-target -march=sm_20 -fopenmp-relocatable-target -save-temps \
-// RUN:   -no-canonical-prefixes %s 2>&1 | FileCheck -check-prefix=CHK-BCLIB %s
+// RUN:   -Xopenmp-target -march=sm_20 --cuda-path=%S/Inputs/CUDA_80/usr/local/cuda \
+// RUN:   -fopenmp-relocatable-target -save-temps -no-canonical-prefixes %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-BCLIB %s
 
 // CHK-BCLIB: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-cuda-bitcode{{.*}}libomptarget-nvptx-sm_20.bc
+// CHK-BCLIB-NOT: No library 'libomptarget-nvptx-sm_20.bc' found in the default clang lib directory or in LIBRARY_PATH. Expect degraded performance due to no inlining of runtime functions on target devices.
 
 /// ###########################################################################
 
 /// Check that the warning is thrown when the libomptarget bitcode library is not found.
 /// Libomptarget requires sm_35 or newer so an sm_20 bitcode library should never exist.
 // RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
-// RUN:   -Xopenmp-target -march=sm_20 -fopenmp-relocatable-target -save-temps \
-// RUN:   -no-canonical-prefixes %s 2>&1 | FileCheck -check-prefix=CHK-BCLIB-WARN %s
+// RUN:   -Xopenmp-target -march=sm_20 --cuda-path=%S/Inputs/CUDA_80/usr/local/cuda
+// RUN:   -fopenmp-relocatable-target -save-temps -no-canonical-prefixes %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-BCLIB-WARN %s
 
 // CHK-BCLIB-WARN: No library 'libomptarget-nvptx-sm_20.bc' found in the default clang lib directory or in LIBRARY_PATH. Expect degraded performance due to no inlining of runtime functions on target devices.