[openmp] Accept directory for libomptarget-bc-path

[JonChesterfield]

Summary

The commandline flag to specify a particular openmp devicertl library
currently errors like:

fatal error: cannot open file
      './runtimes/runtimes-bins/openmp/libomptarget':
      Is a directory

CommonArgs successfully appends the directory to the commandline args then
mlink-builtin-bitcode rejects it.

This patch is a point fix to that. If --libomptarget-amdgcn-bc-path=directory
then append the expected name for the current architecture and go on as before.
This is useful for test runners that don't hardcode the architecture.

[jdoerfert]

LG, but please add a test for this.

[JonChesterfield]

In D109057#2977084, @jdoerfert wrote:

LG, but please add a test for this.

D109061 sends everything in check-openmp down this code path, but we should indeed have a check for the file and for the dir version. Will take a look

walked back through git to find https://reviews.llvm.org/D95161, should be able to do something similar to the tests added there

[JonChesterfield]

Well, that's not helpful. Managed to add a test through the webui as arcanist was misbehaving but it has worked by deleting the functional change. Will try again

[JonChesterfield]

OK, test added, same code as originally, checked that the test fails on trunk. Because it's driver only we don't get a far as the fatal error, but you can see the driver passing the directory through to builtin-bitcode. Will land when the premerge checks catch up.

[thakis]

This breaks tests on windows: http://45.33.8.238/win/44930/step_7.txt

Please take a look and revert if it takes a while to fix. Probably just need to optionally accept win slashiness in the test.

[JonChesterfield]

In D109057#2977632, @thakis wrote:

This breaks tests on windows: http://45.33.8.238/win/44930/step_7.txt

Please take a look and revert if it takes a while to fix. Probably just need to optionally accept win slashiness in the test.

Ah, that explains the {{.*}} patterns. Will fix by dropping the 'libomptarget/' string as the important part is that it found the sm_35.bc in that directory. Commit 88511f6bc56792b47

[JonChesterfield]

Bad times. Changing that to accept the / was a shotgun fix for the problem but it collides with one of the nearby regex, reverting while I work out what is going on. Acceptable fix for linux is to replace the / with {{.}}, as that avoids the problem of two adjacent filecheck matches looking for the same string. I expect that to fix windows too, but will wait for the linked bot above to go through clean first.

[JonChesterfield]

Turns out {{.}} is also fine as far as Linux is concerned and unacceptable to Windows. I can't run anything on Windows locally, and trial and error while watching http://45.33.8.238/win has taken too many iterations already, so I've dropped the test case in 06cdf48a0d94749a19b0b.

[jdoerfert]

In D109057#2977859, @JonChesterfield wrote:

Turns out {{.}} is also fine as far as Linux is concerned and unacceptable to Windows. I can't run anything on Windows locally, and trial and error while watching http://45.33.8.238/win has taken too many iterations already, so I've dropped the test case in 06cdf48a0d94749a19b0b.

@Meinersbur can you take a look?

[Meinersbur]

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

178

-### escapes backslashes in Windows paths UNIX-style (which actually is unnecessary/wrong, the cmd.exe escape character is ^, but windows also ignores consecutive slashes), so the actual output is

"-mlink-builtin-bitcode" "[...]\\clang\\test\\Driver/Inputs/libomptarget\\libomptarget-nvptx-sm_35.bc"

The regex {{.}} does not capture the double backslash. {{/|\\\\}} would work.

[JonChesterfield]

Nice, thanks! I'll wait for the main CI to come back up before trying that

Works on linux as expected, landed - now watching http://45.33.8.238/summary.html

regex works, thanks!

clang/lib/Driver/ToolChains/CommonArgs.cpp

void tools::addOpenMPDeviceRTL(const Driver &D,
   llvm::sys::path::append(DefaultLibPath, Twine("lib") + CLANG_LIBDIR_SUFFIX);
   LibraryPaths.emplace_back(DefaultLibPath.c_str());
 
   OptSpecifier LibomptargetBCPathOpt =
       Triple.isAMDGCN() ? options::OPT_libomptarget_amdgcn_bc_path_EQ
                         : options::OPT_libomptarget_nvptx_bc_path_EQ;
 
   StringRef ArchPrefix = Triple.isAMDGCN() ? "amdgcn" : "nvptx";
+  std::string LibOmpTargetName = "libomptarget-" + BitcodeSuffix.str() + ".bc";
+
   // First check whether user specifies bc library
   if (const Arg *A = DriverArgs.getLastArg(LibomptargetBCPathOpt)) {
-    std::string LibOmpTargetName(A->getValue());
-    if (llvm::sys::fs::exists(LibOmpTargetName)) {
+    SmallString<128> LibOmpTargetFile(A->getValue());
+    if (llvm::sys::fs::exists(LibOmpTargetFile) &&
+        llvm::sys::fs::is_directory(LibOmpTargetFile)) {
+      llvm::sys::path::append(LibOmpTargetFile, LibOmpTargetName);
+    }
+
+    if (llvm::sys::fs::exists(LibOmpTargetFile)) {
       CC1Args.push_back("-mlink-builtin-bitcode");
-      CC1Args.push_back(DriverArgs.MakeArgString(LibOmpTargetName));
+      CC1Args.push_back(DriverArgs.MakeArgString(LibOmpTargetFile));
     } else {
       D.Diag(diag::err_drv_omp_offload_target_bcruntime_not_found)
-          << LibOmpTargetName;
+          << LibOmpTargetFile;
     }
   } else {
     bool FoundBCLibrary = false;
 
-    std::string LibOmpTargetName =
-        "libomptarget-" + BitcodeSuffix.str() + ".bc";
-
     for (StringRef LibraryPath : LibraryPaths) {
       SmallString<128> LibOmpTargetFile(LibraryPath);
       llvm::sys::path::append(LibOmpTargetFile, LibOmpTargetName);
       if (llvm::sys::fs::exists(LibOmpTargetFile)) {
         CC1Args.push_back("-mlink-builtin-bitcode");
         CC1Args.push_back(DriverArgs.MakeArgString(LibOmpTargetFile));
         FoundBCLibrary = true;
         break;
       }
     }
 
     if (!FoundBCLibrary)
       D.Diag(diag::err_drv_omp_offload_target_missingbcruntime)
           << LibOmpTargetName << ArchPrefix;
   }
 }

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

 // RUN:   -fopenmp-relocatable-target -fopenmp-target-new-runtime -save-temps -no-canonical-prefixes %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-BCLIB-NEW %s
 /// The user can override default detection using --libomptarget-nvptx-bc-path=.
 // RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
 // RUN:   --libomptarget-nvptx-bc-path=%S/Inputs/libomptarget/libomptarget-nvptx-test.bc \
 // RUN:   -Xopenmp-target -march=sm_35 --cuda-path=%S/Inputs/CUDA_102/usr/local/cuda \
 // RUN:   -fopenmp-relocatable-target -save-temps -no-canonical-prefixes %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-BCLIB-USER %s
+/// The user can also pass the path to the directory containing the bitcode lib
+// RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
+// RUN:   --libomptarget-nvptx-bc-path=%S/Inputs/libomptarget \
+// RUN:   -Xopenmp-target -march=sm_35 --cuda-path=%S/Inputs/CUDA_102/usr/local/cuda \
+// RUN:   -fopenmp-relocatable-target -save-temps -no-canonical-prefixes %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-BCLIB-USER-DIR %s
 
 // CHK-BCLIB: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget-nvptx-sm_35.bc
 // CHK-BCLIB-NEW: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget-new-nvptx-sm_35.bc
 // CHK-BCLIB-USER: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget-nvptx-test.bc
-
+// CHK-BCLIB-USER-DIR: clang{{.*}}-triple{{.*}}nvptx64-nvidia-cuda{{.*}}-mlink-builtin-bitcode{{.*}}libomptarget{{.}}libomptarget-nvptx-sm_35.bc
 // CHK-BCLIB-NOT: {{error:|warning:}}
 
 /// ###########################################################################
 
 /// Check that the warning is thrown when the libomptarget bitcode library is not found.
 /// Libomptarget requires sm_35 or newer so an sm_35 bitcode library should never exist.
 // RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda \
 // RUN:   -Xopenmp-target -march=sm_35 --cuda-path=%S/Inputs/CUDA_102/usr/local/cuda \