Page MenuHomePhabricator

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

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



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


Event Timeline

rdhindsa created this revision.May 8 2018, 3:25 PM
ruiu added a comment.May 8 2018, 5:51 PM

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

817 ↗(On Diff #145801)

Please also add "but got" part.

1028 ↗(On Diff #145801)

Remove llvm::

269 ↗(On Diff #145801)

Remove llvm::

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

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.May 9 2018, 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.May 9 2018, 12:33 PM
rdhindsa updated this revision to Diff 145986.May 9 2018, 12:35 PM
ruiu added inline comments.May 9 2018, 12:51 PM
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.May 9 2018, 1:04 PM
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.May 9 2018, 1:32 PM
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.May 10 2018, 5:44 AM
ruiu added inline comments.May 10 2018, 11:21 AM
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.May 10 2018, 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?

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.May 11 2018, 12:30 PM
rdhindsa updated this revision to Diff 146411.May 11 2018, 2:04 PM
ruiu added inline comments.May 14 2018, 3:56 PM
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.

1037 ↗(On Diff #146411)

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

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.May 16 2018, 12:56 PM
rdhindsa added inline comments.May 16 2018, 12:59 PM
822–823 ↗(On Diff #146411)

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

ruiu accepted this revision.May 16 2018, 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.

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.May 16 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.