Thanks, this is great!

There is this vague, unspecified idea across the code base not to "touch" anything that hasSection(). We should be more deliberate and decide on a case-by-case basis, so this looks like the right thing to do for this pass.

@pcc, @tejohnson: Are you OK with this going in now that D47898 has merged?

• Rebase for current tree
• Fix -flto -save-temps which the previous patch broke

In D34156#1125489, @vlad.tsyrklevich wrote:

Hi Tobias, I tracked down the failure self-hosting LLVM with LTO with this revision to https://bugs.llvm.org/show_bug.cgi?id=37684#c2 and have a fix under review in D47898.

In D34156#781415, @pcc wrote:

Please confirm that we can still self host with full LTO now that D33922 has landed.

In D34156#780714, @mehdi_amini wrote:

In D34156#780570, @tobiasvk wrote:

• Change patch to always emit a module summary for regular LTO

I don't see any real downside of having a summary given the marginal time and
space overhead it takes to construct and save it.

I think some code/logic needs to be changed in LLVM first:

bool LTOModule::isThinLTO() {
  // Right now the detection is only based on the summary presence. We may want
  // to add a dedicated flag at some point.

• Change patch to always emit a module summary for regular LTO

In D34156#779270, @pcc wrote:

Have you considered writing the regular LTO summaries unconditionally if -flto was specified? That was how I imagined that the interface would look.

In D32819#749163, @rnk wrote:

One question remains: do you think I should completely encapsulate this indexing change by adding 1 inside AttributeList::getAttributes, or is it better to update all the callers as I did in this change? The problem with this change is that it will break out-of-tree backends and frontends that use the APIs with one-based indexing. It won't even be a compile failure because the APIs still take unsigned integers. That's a bit rude without some more hand waving on llvm-dev and release notes.

This is very useful, thanks for working on it!

Add test case now that D31920 is merged.

In D31901#723155, @pcc wrote:

Agreed with @davide that this needs a test case. I am working on a change to llvm-lto2 that adds a feature for dumping the LTO symbol table, which should allow you to test this. @tobiasvk would you be fine with waiting until that change lands?

Any further comments? I'd like to go ahead and merge this.

Add Rafael's suggested disclaimer to the test case.

Rebase and update test case comment.

Make Gerolf's suggested changes to comments.

In D31443#712679, @Gerolf wrote:

I'd love to see some comments an get your thoughts about a verifier.

Ping?

Move test to test/LTO/Resolution as per Peter's request.

In D29475#665358, @davide wrote:

ehm, we already have -save-temps, why do you need this? (see what the gold-plugin and lld do)

In D29475#665351, @mehdi_amini wrote:

Can you clarify how one would implement -save-temps in a linker this way?

This makes sense. I shared our own similar function merging pass (in D22051), which is more generic than the Swift one, and it could potentially also use this infrastructure. We could go a little further and pull out more of the stuff in MergeFunctions that they all share (e.g. thunk creation, block traversal) in the future.

Thanks David for the quick review - agree with most of your points, some comments added.

In D22051#476003, @jfb wrote:

Could you detail how this is different from MergeFunc, and what it would take to add the new capabilities to MergeFunc instead of duplicating?

Can this go in please? This is breaking debug builds for a number of critical codes for us.

We hit this bug too and your fix works for our test cases. LGTM, thanks!

In D16124#328371, @pcc wrote:

LGTM with this one change. Thanks!

In D16124#325170, @pcc wrote:

As far as I can tell, it seems that in a typical LTO scenario with internalization, this partitioning would be strongly biased towards a single partition containing most of the code. Do you have a plan to address this?

The most common uses cases for explicit sections I'm aware of here are situations where the user wants to place part of the code into special memory (e.g. tightly-coupled memory). You're free to optimize within that special section - in fact, that's even desired because the code tends to be very performance-critical (but space-constrained) - but introducing additional control or data edges to other sections is a no-go (which is what we're currently doing in GlobalOpt). The proposed patch would solve the problem nicely.

LGTM conceptually. The patch doesn't apply to the current repo though (I guess it's a diff against what you had locally).

I just did

Updated patch with Peter and Mehdi's comments - thank you both!

In D14554#286654, @joker.eph wrote:

The bitcode->assembly part is available with llc though, not sure why it's not enough?

Ah I wasn't aware of the llvm-split utility - makes sense. It would still be useful to get bitcode out of llvm-lto for testing. But that could be built on top of writeMergedModule, as you mentioned.

Update patch to no longer create temporary instructions

Change patch to use a load of undef, instead of just an undef, as the index for
the temporary GEP. This should make it safe even if aliasGEP ever gets smarter
about undefs.

Hi Hal, Daniel,

Remove spurious extra directories that ended up in the patch.

Add a flag to guard this change and leave it off by default for now.

Hi Daniel, Hal,

Rebased