[PATCH] D23015: [ThinLTO/gold] Conservatively handle unknown GVs when internalizing

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23015#503045, @mehdi_amini wrote:

> In https://reviews.llvm.org/D23015#503013, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D23015#502970, @mehdi_amini wrote:
> >
> > > OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
> > >  Then why not changing this?
> >
> >
> > We use gold's symbol resolution to determine which values to link in. So in the example, it knows that f1 is a preempted symbol in %t2.o and doesn't need to include its definition. I don't think we should need to change that.
>
>
> I see...
>  I still does not think that's a great thing: using the `IRMover` just to get a module where some symbols are pruned seems quite sketchy to me.
>
> Here we accept that the module can have renamed symbols from what is in the index, but that should be an invariant or we'll run in trouble at some point. So I'm not convinced the fix is right at this point.


I decided I agree, see below.

> I dug a bit more, I'm still trying to follow how the bug happens: the IRMover is forcing `@f1` (in Inputs/thinlto_alias2.ll) to be linked to satisfy the alias `@a2`. And since there is already a declaration for `@f1` when it does so, it is supposed to create a local copy (i.e. it will be an *internal* function), IRMover.cpp ~line 930.


Correct.

> Now the code you are patching is a callback for the internalize utility, and IIUC it is not called on internal function (which I expect to be the case here), as I read `InternalizePass::shouldPreserveGV`.


It has been promoted though (by renameModuleForThinLTO), so it is not internal at this point (which is why the CHECK in the new test case looks like "CHECK: define hidden i32 @f1.1.llvm.0"

But after I sent this last night I realized that we probably should not have the gold-plugin drop any preempted symbols at this point. The reason is that we are losing optimization opportunities:

1. Loss of inlining opportunity for the dropped preempted linkonce_odr: In this case the weak_odr can't be inlined anyway, but if it was linkonce_odr, we wouldn't be able to inline it if it was preempted (we would have to import it back in, but the import decisions will be made when we still have the linkonce_odr definition and the calls wouldn't be external, but in any case there isn't any point in dropping only to import for inlining later).
2. Loss of internalization for the local copy - in this case we are checking to see if we can internalize the promoted symbol, which we would be able to do in this case if we could locate its summary. So we lose this opportunity.

I have made a change to the gold-plugin to keep the preempted symbols as well. However, this exposed another issue: when we keep the preempted linkonce in the comdat, thinLTOResolveWeakForLinkerModule comes along and changes it to available_externally. We then get a verification failure about a declaration in a comdat.

Interestingly, we have a fix for this in renameModuleForThinLTO, which drops any available_externally from comdats. It is meant to clean up after importing, but if I reorder renameModuleForThinLTO and thinLTOResolveWeakForLinkerModule in gold-plugin, it fixes this particular issue. I managed to reproduce this issue with an llvm-lto test case, since ProcessThinLTOModule() in ThinLTOCodeGenerator.cpp has the same ordering. It is also fixed by flipping the ordering between these two. Interestingly, and probably completely accidentally, ThinLTOCodeGenerator::promote() already has the desirable ordering. However, I don't like the subtle ordering requirement between these two calls. Perhaps it is better to explicitly strip any new available_externally symbols from comdats in thinLTOResolveWeakForLinkerModule. Hmm, I think I like that approach better. WDYT? Let me do that, will do so and clean up and add the new test cases to the patches later today.


https://reviews.llvm.org/D23015