This is an archive of the discontinued LLVM Phabricator instance.

[LinkerWrapper] Rework the linker wrapper and use owning binaries
ClosedPublic

Authored by jhuber6 on Jun 7 2022, 1:03 PM.

Details

Summary

The linker wrapper currently eagerly extracts all identified offloading
binaries to a file. This isn't ideal because we will soon open these
files again to examine their symbols for LTO and other things.
Additionally, we may not use every extracted file in the case of static
libraries. This would be very noisy in the case of static libraries that
may contain code for several targets not participating in the current
link.

Recent changes allow us to treat an Offloading binary as a standard
binary class. So that allows us to use an OwningBinary to model the
file. Now we keep it in memory and only write it once we know which
files will be participating in the final link job. This also reworks a
lot of the structure around how we handle this by removing the old
DeviceFile class.

The main benefit from this is that the following doesn't output 32+ files and
instead will only output a single temp file for the linked module.

$ clang input.c -fopenmp --offload-arch=sm_70 -foffload-lto -save-temps

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 7 2022, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 1:03 PM
jhuber6 requested review of this revision.Jun 7 2022, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 1:03 PM
jhuber6 updated this revision to Diff 435118.Jun 8 2022, 5:59 AM

There was a problem where the Triple and Arch data would be deallocated when the LTO pass took ownership of every single file. Add a UniqueStringSaver to make sure they are still accessible after linking.

jhuber6 updated this revision to Diff 435122.Jun 8 2022, 6:22 AM

Add use of bitcode libraries so this works on AMD.

jhuber6 updated this revision to Diff 435124.Jun 8 2022, 6:28 AM

Sorry for this noise. This is a pretty large change but shouldn't affect any functionality and passes all the tests I know of, so this should be good to land. Let me know if you have any objections to how I've structured this.

jhuber6 updated this revision to Diff 435571.Jun 9 2022, 8:41 AM

Fixing bug when capturing a StringRef by reference in a callback.

Is anyone up to review this? I'm mostly looking for some feedback on the interfaces I've built. If no one has time to look into it I can probably just land without review.

jhuber6 updated this revision to Diff 438758.Jun 21 2022, 10:48 AM

Adding a test to ensure we no longer write temporary files for unused inputs.

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?

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
338

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

784

Why call out Arch here? Passing a StringRef by reference via the & should be fine

903

Existing comment I see, but what justifies that assumption?

1136

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

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.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
338

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)
784

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.

903

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.

1136

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.

JonChesterfield accepted this revision.Jun 21 2022, 12:25 PM

I think this is probably OK. Smaller patches usually get reviewed faster so minimising the line noise in the browser is worthwhile.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
784

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.

1136

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

This revision is now accepted and ready to land.Jun 21 2022, 12:25 PM