jhuber6 added a comment.
In D127246#3599826 <https://reviews.llvm.org/D127246#3599826>, @JonChesterfield
wrote:
> I've read it but can't promise it's correct - the diff is large and has some
> spurious noise in it which distracts significantly from the functional
> changes.
>
> Would you be willing to split this into two patches, one which renames
> variables and moves blocks of code around without changing them and a second
> that has the functional changes?
It wouldn't be easy, it rewrites a lot of the internal logic. I could've made a
patch that was slightly less invasive, but I was going to change it anyway so I
didn't want to write code that I was just going to delete.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:338
+ Contents.getBufferIdentifier());
+ auto NewBinaryOrErr = OffloadBinary::create(*BufferCopy);
+ if (!NewBinaryOrErr)
----------------
JonChesterfield wrote:
> This is syntactically a bit weird: `std::move(*NewBinaryOrErr)`
>
> Could you spell out the type? I think based on the previous version it's
> probably an Expected<unique_ptr<>> but my first thought was whether a
> unique_ptr was being heap allocated by ::create
All of the `Binary` functions have the same signature for creating them. It
might help to make this more explicit so I'll do that.
```
Expected<uniqute_ptr<T>> T::create(MemoryBufferRef)
```
================
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;
----------------
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.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:903
// We assume visibility of the whole program if every input file was bitcode.
+ bool WholeProgram = InputFiles.empty();
----------------
JonChesterfield wrote:
> Existing comment I see, but what justifies that assumption?
We don't support shared libraries for offloading, so we know every single input
file at link time. That's why we can assert we see everything if every single
input is bitcode.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1136
+
+ Triple TheTriple = Triple(Images.front().StringData.lookup("triple"));
+ auto FileOrErr = nvptx::fatbinary(InputFiles, TheTriple);
----------------
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.
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
