[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

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

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

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

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

[ABataev]

lib/Driver/ToolChains/Cuda.cpp

535

1. char *->const char *
2. ::getenv->llvm::Process::GetEnv

539

Use llvm::SplitString instead

548

const std::string &

[gtbercea]

Address comments.

[ABataev]

lib/Driver/ToolChains/Cuda.cpp

539

Maybe just LibraryPaths.emplace_back(DefaultLibPath);?

546

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

200–201

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

lib/Driver/ToolChains/Cuda.cpp

534

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

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

[Hahnfeld]

lib/Driver/ToolChains/Cuda.cpp

536–542

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

[ABataev]

lib/Driver/ToolChains/Cuda.cpp

540

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

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

540

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

543–544

Move the definition of LibPath out of if statement scope.

548

auto->StringRef

555

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.

include/clang/Basic/DiagnosticDriverKinds.td

def err_drv_omp_host_ir_file_not_found : Error<
   "The provided host compiler IR file '%0' is required to generate code for OpenMP target regions but cannot be found.">;
 def err_drv_omp_host_target_not_supported : Error<
   "The target '%0' is not a supported OpenMP host target.">;
 def err_drv_expecting_fopenmp_with_fopenmp_targets : Error<
   "The option -fopenmp-targets must be used in conjunction with a -fopenmp option compatible with offloading, please use -fopenmp=libomp or -fopenmp=libiomp5.">;
 def warn_drv_omp_offload_target_duplicate : Warning<
   "The OpenMP offloading target '%0' is similar to target '%1' already specified - will be ignored.">, 
   InGroup<OpenMPTarget>;
+def remark_drv_omp_offload_target_missingbcruntime : Warning<
+  "Expect degraded performance on the target device due to missing '%0' in LIBRARY_PATH.">,
+  InGroup<OpenMPTarget>;
 def err_drv_bitcode_unsupported_on_toolchain : Error<
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
 
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
 def warn_drv_optimization_value : Warning<"optimization level '%0' is not supported; using '%1%2' instead">,
   InGroup<InvalidCommandLineArgument>;
 def warn_ignored_gcc_optimization : Warning<"optimization flag '%0' is not supported">,
   InGroup<IgnoredOptimizationArgument>;

lib/Driver/ToolChains/Cuda.cpp

if (CudaInstallation.version() >= CudaVersion::CUDA_90) {
     CC1Args.push_back("+ptx60");
   } else {
     // Libdevice in CUDA-7.0 requires PTX version that's more recent
     // than LLVM defaults to. Use PTX4.2 which is the PTX version that
     // came with CUDA-7.0.
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+ptx42");
   }
+
+  if (DeviceOffloadingKind == Action::OFK_OpenMP) {
+    SmallVector<std::string, 8> LibraryPaths;
+    if (char *env = ::getenv("LIBRARY_PATH")) {
+      StringRef CompilerPath = env;
+      while (!CompilerPath.empty()) {
+        std::pair<StringRef, StringRef> Split =
+            CompilerPath.split(llvm::sys::EnvPathSeparator);
+        LibraryPaths.push_back(Split.first);
+        CompilerPath = Split.second;
+      }
+    }
+
+    std::string LibOmpTargetName =
+      "libomptarget-nvptx-" + GpuArch.str() + ".bc";
+    bool FoundBCLibrary = false;
+    for (std::string LibraryPath : LibraryPaths) {
+      SmallString<128> LibOmpTargetFile(LibraryPath);
+      llvm::sys::path::append(LibOmpTargetFile, LibOmpTargetName);
+      if (llvm::sys::fs::exists(LibOmpTargetFile)) {
+        CC1Args.push_back("-mlink-cuda-bitcode");
+        CC1Args.push_back(DriverArgs.MakeArgString(LibOmpTargetFile));
+        FoundBCLibrary = true;
+        break;
+      }
+    }
+    if (!FoundBCLibrary)
+      getDriver().Diag(diag::remark_drv_omp_offload_target_missingbcruntime)
+          << LibOmpTargetName;
+  }
 }
 
 void CudaToolChain::AddCudaIncludeArgs(const ArgList &DriverArgs,
                                        ArgStringList &CC1Args) const {
   // Check our CUDA version if we're going to include the CUDA headers.
   if (!DriverArgs.hasArg(options::OPT_nocudainc) &&
       !DriverArgs.hasArg(options::OPT_no_cuda_version_check)) {
     StringRef Arch = DriverArgs.getLastArgValue(options::OPT_march_EQ);

test/Driver/openmp-offload-gpu.c

 
 /// Check that error is not thrown by toolchain when no cuda lib device is found when using -S.
 /// Check that the flag is passed when -fopenmp-relocatable-target is used.
 // RUN:   %clang -### -S -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 \
 // RUN:   -fopenmp-relocatable-target -save-temps -no-canonical-prefixes %s 2>&1 \
 // 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 that will be found via the LIBRARY_PATH.
+// RUN:   touch /tmp/libomptarget-nvptx-sm_60.bc
+// RUN:   LIBRARY_PATH=/tmp %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
+// RUN:   -Xopenmp-target -march=sm_60 -fopenmp-relocatable-target -save-temps \
+// RUN:   -no-canonical-prefixes %s 2>&1 | FileCheck -check-prefix=CHK-BCLIB %s
+
+// CHK-BCLIB: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-cuda-bitcode{{.*}}libomptarget-nvptx-sm_60.bc
+
+/// ###########################################################################
+
+/// 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:   LIBRARY_PATH= %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
+
+// CHK-BCLIB-WARN: Expect degraded performance on the target device due to missing 'libomptarget-nvptx-sm_20.bc' in LIBRARY_PATH.