[PATCH] D96877: [libomptarget] Try a fallback devicertl if the preferred one is missing

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1647
+    llvm::Twine lib) {
+  for (StringRef LibraryPath : LibraryPaths) {
+    SmallString<128> TargetFile(LibraryPath);
----------------
^clang-tidy warning about a twine in a parameter list [llvm-twine-local] looks like a bug


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1695
 
-    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;
+    llvm::Twine LibOmpTargetName = "libomptarget-" + BitcodeSuffix + ".bc";
+    llvm::Twine FallbackTargetName =
----------------
tianshilei1992 wrote:
> If you're using `Twine`, `+` is not needed.
> ```
> llvm::Twine LibOmpTargetName("libomptarget-", BitcodeSuffix ,".bc");
> ```
This doesn't appear to be the case. Doesn't compile anyway, no matching constructor.

The discussion around the clang-tidy warning states that Twine variables shouldn't be used as local variables because they don't make copies of the arguments, so one can hit use after free. I don't agree with that, but it doesn't matter very much so have changed to std::string.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1697
+    llvm::Twine FallbackTargetName =
+        "libomptarget-" + ArchPrefix + "-unknown.bc";
+
----------------
tianshilei1992 wrote:
> The `BitcodeSuffix` for NVPTX consists of three parts: `nvptx`, `cuda_xxx`, and `sm_yy`. We might want to have an unknown for every `sm`?
Lets not go there. 'Unknown' is a reasonably strong indicator for 'this might not work'. One per sm suggests we've somehow tested whether it's going to be OK or not.

I'd prefer we stick with 'using a cuda that didn't exist when your clang shipped is a bad idea, get a newer clang or an older cuda'. This is a sketch of how we could go the other way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96877/new/

https://reviews.llvm.org/D96877