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