JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.
I think this is probably OK. Smaller patches usually get reviewed faster so
minimising the line noise in the browser is worthwhile.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:784
};
- Conf.PostInternalizeModuleHook = [&](size_t, const Module &M) {
+ Conf.PostInternalizeModuleHook = [&, Arch](size_t, const Module &M) {
SmallString<128> TempFile;
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > Why call out Arch here? Passing a StringRef by reference via the & should
> > be fine
> It apparently wasn't fine. For whatever reason when this function was called
> by the LTO engine this value was getting mutated and pointing to bad memory.
> If I capture it by-value everything is fine. I didn't care enough to look
> into it once I figured out the fix.
Interesting. The filename operator+ behaving differently for StringRef vs
StringRef& seems suspicious. Passing by std::string is presumably safer again,
but I guess this will do for now.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1136
+
+ Triple TheTriple = Triple(Images.front().StringData.lookup("triple"));
+ auto FileOrErr = nvptx::fatbinary(InputFiles, TheTriple);
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > This is close but not quite a copy of existing code, moving it around in
> > the file at the same time as changing it makes the diff significantly
> > harder to follow
> Sorry, I don't use a header file here so it's a little bit of a pain to make
> sure new functions can call the ones they need. This function is
> fundamentally different from the old one so I wouldn't worry about the old
> implementations.
You could probably move the old functions further up in the file as a precommit
change so that they lined up in the functional diff
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127246/new/
https://reviews.llvm.org/D127246
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
