Add support for ThinLTO plugin option thinlto-object-suffix-replace
ClosedPublic

Authored by rdhindsa on Tue, May 8, 3:25 PM.

Details

Summary

Add support for ThinLTO plugin option thinlto-object-suffix-replace

thinlto-object-suffix-replace is an option to control the name of modules encoded in the individual index files for a distributed backend. This enables the use of minimized bitcode files for the thin link, assuming the name of the full bitcode file used in the backend differs just in some part of the file suffix.

In this patch, we are updating obj path names by consuming old suffix from the back and appending new suffix.

Diff Detail

Repository
rL LLVM
rdhindsa created this revision.Tue, May 8, 3:25 PM
ruiu added a comment.Tue, May 8, 5:51 PM

Could you explain what this option is? I cannot get what is expected for that option from this patch.

lld/ELF/Driver.cpp
817 ↗(On Diff #145801)

Please also add "but got" part.

lld/ELF/InputFiles.cpp
1028 ↗(On Diff #145801)

Remove llvm::

lld/ELF/LTO.cpp
269 ↗(On Diff #145801)

Remove llvm::

(You probably should other code in the same file and write code in the same way as others.)

lld/test/ELF/lto/thinlto_object_suffix_replace.ll
1 ↗(On Diff #145801)

We use - instead of - in test file name.

18 ↗(On Diff #145801)

Please split a long line into shorter lines and concatenate them with \.

rdhindsa updated this revision to Diff 145985.Wed, May 9, 12:31 PM
rdhindsa edited the summary of this revision. (Show Details)

Updated the summary for option description.

rdhindsa marked 5 inline comments as done.Wed, May 9, 12:33 PM
rdhindsa updated this revision to Diff 145986.Wed, May 9, 12:35 PM
ruiu added inline comments.Wed, May 9, 12:51 PM
lld/ELF/InputFiles.cpp
1028 ↗(On Diff #145986)

I don't think we should add new tricky code to this common place. Instead, we should modify LTO.cpp or somewhere else. Let me try to do that locally.

ruiu added inline comments.Wed, May 9, 1:04 PM
lld/ELF/InputFiles.cpp
1030–1033 ↗(On Diff #145986)

All tests including the new one you are adding in this patch seem to pass without these four lines of code. Is this needed?

rdhindsa added inline comments.Wed, May 9, 1:32 PM
lld/ELF/InputFiles.cpp
1030–1033 ↗(On Diff #145986)

Yeah it is needed as this handles all the bitcode files except lazyobjfiles. I verified that the test thinlto-object-suffix-replace fails without these changes.

grimar added a subscriber: grimar.Thu, May 10, 5:44 AM
ruiu added inline comments.Thu, May 10, 11:21 AM
lld/ELF/InputFiles.cpp
1030–1033 ↗(On Diff #145986)

Yeah, I verified that your test would fail without this piece of code. But it doesn't feel right to add a new piece of this common path just for a minor ThinLTO feature. That seems too intrusive to me. It might be unavoidable, but we probably shouldn't do this. Let me actually modify this code.

ruiu added a comment.Thu, May 10, 4:00 PM

I tried a few different ideas but I couldn't come up with a better idea than this patch, so this seems like a best way of doing this so far. Could you please update the patch with my comments?

lld/ELF/InputFiles.cpp
1028 ↗(On Diff #145986)

StringRef -> std::string and then remove UpdatedPath. You can mutate Path if you change its type to std::string.

1031–1032 ↗(On Diff #145986)

If Path doesn't end with first, then it doesn't remove anything from its end, and then second is added to Path. I doubt it is a correct behavior.

rdhindsa updated this revision to Diff 146380.Fri, May 11, 12:30 PM
rdhindsa updated this revision to Diff 146411.Fri, May 11, 2:04 PM
ruiu added inline comments.Mon, May 14, 3:56 PM
lld/ELF/Driver.cpp
822–823 ↗(On Diff #146411)

nit: it is perhaps better to split the long literal string into two like this

error("thinlto-object-suffix-replace expects 'old;new' format, "
      "but got " + S.substr(30));

so that the code looks better.

lld/ELF/InputFiles.cpp
1037 ↗(On Diff #146411)

I don't think this is correct. Don't you need to use ends_with?

lld/ELF/LTO.cpp
270–278 ↗(On Diff #146411)

The amount of code duplication is growing, and you probably should do something to against it.

rdhindsa updated this revision to Diff 147162.Wed, May 16, 12:56 PM
rdhindsa added inline comments.Wed, May 16, 12:59 PM
lld/ELF/Driver.cpp
822–823 ↗(On Diff #146411)

clang-format updates by splitting (" but got " + S.substr(30));) to two lines .

ruiu accepted this revision.Wed, May 16, 1:20 PM

There are a few things that can be improved in this patch, but I think that changing everything before submitting is not necessary. Please go ahead and submit. I'll make a follow-up change after that.

lld/ELF/Driver.cpp
822–823 ↗(On Diff #146411)

This is much uglier than before, but I'd think we should fight against clang-format. I'd just concatenate the two strings into one literal string just like before.

This revision is now accepted and ready to land.Wed, May 16, 1:20 PM
This revision was automatically updated to reflect the committed changes.