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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
