This is an archive of the discontinued LLVM Phabricator instance.

ThinLTO: Resolve linkonce_odr aliases just like functions
ClosedPublic

Authored by mehdi_amini on Apr 19 2016, 10:20 PM.

Details

Summary

D19096 has drifted too much, I'm trying to isolate smaller pieces.
Hopefully this one make sense in isolation.

This help to streamline the process of handling importing since
we don't need to special case alias everywhere: just like
linkonce_odr we are sure we will emit one copy.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to ThinLTO: Resolve linkonce_odr aliases just like functions.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added a subscriber: llvm-commits.
tejohnson added inline comments.Apr 20 2016, 9:06 AM
lib/LTO/ThinLTOCodeGenerator.cpp
161

Since this is a variant of similar handling that used to be here for the aliasee in D19096 (since removed), I will copy over a variant of my suggestion from back then:

I think it would be helpful to add a comment here about how this works. I.e. something like "If a non-imported alias that remains linkonce is referenced by an imported function, the IRMover will create a corresponding ExternalLinkage declaration, which will be satisfied at link time with the copy that was resolved to WeakODR."

mehdi_amini added inline comments.Apr 20 2016, 12:13 PM
lib/LTO/ThinLTOCodeGenerator.cpp
161

Your sentence is not clear to me, I'm not sure if I misunderstand because it does not seems to apply. See also line 170 for the general explanation that seems to cover what happens here (nothing is really specific to alias for the resolution).

The only specificity here with alias is we can't turn them into available_externally.
We turn ODR functions (even if they have a callee remaining in the current module) into available externally to *force* dropping them.

tejohnson edited edge metadata.Apr 20 2016, 12:18 PM

Were you going to move your old test/ThinLTO/X86/alias_resolution.ll from D19096 to here?

lib/LTO/ThinLTOCodeGenerator.cpp
161

True, the same explanation is valid for why the available_externally works since it is discardable as well. I think the explanation at line 170 is sufficient.

mehdi_amini edited edge metadata.

Add back the test

Are we OK on this one as well?

tejohnson accepted this revision.Apr 20 2016, 2:26 PM
tejohnson edited edge metadata.

Just a few suggestions on the test case comments.

test/ThinLTO/X86/alias_resolution.ll
8

maybe add ", since there is a single copy of each, and they are not exported and discardable" for clarity

35

Shouldn't this be:
"Only linkonce_odr alias is resolved in the case when there is a single copy"
I guess this depends on what it means to be "resolved". I took "resolved" to mean we decided which copy to select, which isn't the case for most aliases.

50

The word "resolved" here seems contradictory to how it is being used in the rest of the patch. Maybe change this to something like:
" Only import aliases that will not become available_externally, i.e. alias to a linkonce_odr."

This revision is now accepted and ready to land.Apr 20 2016, 2:26 PM
mehdi_amini marked 3 inline comments as done.Apr 20 2016, 3:03 PM
mehdi_amini added inline comments.
test/ThinLTO/X86/alias_resolution.ll
8

The "since there is a single copy of each" part seems irrelevant: this is really about are they exported or not.
And we only import alias when they point to a linkonce_odr function, so these are not exported and won't need to be resolved (I'm not sure if this limitation has any other source than being inherited from the old ModuleLinker).

35

Resolved in the sense of the function ResolveODR:

/// We'd like to drop these function if they are no longer referenced in the
/// current module. However there is a chance that another module is still
/// referencing them because of the import. We make sure we always emit at least
/// one copy.

So it is about the aliases being linkonce *and* being exported, that needs to be turned into weak(_odr).

Are we talking about the same "resolve"?

50

I should write "The current implementation is limited (for no real reasons) to aliases to linkonce_odr", unless there is something I missed...

tejohnson added inline comments.Apr 20 2016, 4:34 PM
test/ThinLTO/X86/alias_resolution.ll
8

The limitation is due to the linkage changes made when we do the importing, isn't it related to this comment in the function importer?:

+ Alias can't point to "available_externally". However when we import
+
linkOnceODR the linkage does not change. So we import the alias
+ and aliasee only in this case.
+
FIXME: we should import alias as available_externally *function*, the
+ // destination module does need to know it is an alias.

35

My point was that some of the above linkonce_odr aliases would also be "resolved" to weak_odr by ResolveODR if there were multiple copies, and they were the first definition for linker. I.e. in this test, the reason why we resolve the below linkonce_odr alias and not the earlier ones is due to the fact that there is a single copy (which is exported).

Also, we're not really resolving anything other than the ODR aliases - the rest are ignored by ResolveODR. So the non linkonce_odr/weak_odr aliases aren't really being resolved are they?

50

This is related to the comment I copied in earlier, it is due to the need to replace alias with copy when importing, due to needing to avoid an alias to available_externally, right? I was suggesting that the reason be documented more specifically in the test (we will want to change the test when we can import more aliases).

mehdi_amini added inline comments.Apr 20 2016, 4:49 PM
test/ThinLTO/X86/alias_resolution.ll
35

You're right!

It means I miss a test to check that with multiple aliases, even without import they are resolved. This is purely "resolve on import".

50

Yes

mehdi_amini added inline comments.Apr 20 2016, 4:55 PM
test/ThinLTO/X86/alias_resolution.ll
39

There is a bug I think, this one should be promoted right?

tejohnson added inline comments.Apr 20 2016, 5:34 PM
test/ThinLTO/X86/alias_resolution.ll
39

You mean resolved? The alias itself isn't ODR so I think it stays Linkonce(Any).

mehdi_amini added inline comments.Apr 20 2016, 6:37 PM
test/ThinLTO/X86/alias_resolution.ll
39

I think the current situation is murky. Let see if I can summarize it right:

  • linkonce (ODR or not) can be discarded: if the symbol is referenced from another module, it needs to be "resolved" for *correctness*.
  • weak (ODR or not) can't be discarded: we don't need to resolve them for *correctness*.
  • linkonce/weak (the *non* ODR variant) can't be inlined, there is probably no interest in importing them, we should disable that in the importing decision.
  • For all of them: we want to resolve when there is more than one definition for compile-time optimization purpose.
tejohnson added inline comments.Apr 20 2016, 10:39 PM
test/ThinLTO/X86/alias_resolution.ll
39

Yes, I think that is a correct summary. So the bug as you point out is in fact that we should be resolving linkonce(any) to weak(any) since it is discardable.

Otherwise if under some situation the call was removed in the original exporting module (not through inlining since that doesn't happen for non-ODR linkonce, but some other transformation), any imported reference that didn't pull in the def can get a linker undef.

So seems we need to do the resolving of linkonce(any) to weak(any) if it is exported. And could do it when there is more than one definition as a compile time optimization.

I remember that in an older version of the patch you were doing this resolution for the non-odr types as well, but I don't remember why you stopped doing that for the WeakAny and LinkOnceAny. Was there an issue that I am not remembering?

mehdi_amini added inline comments.Apr 20 2016, 10:42 PM
test/ThinLTO/X86/alias_resolution.ll
39

I don't remember the history unfortunately.

What I am gonna do is moving forward with this patch, leaving the bug as-is, since it is already existing I think.
I'll implement in a new patch (or a few patches) the proper handling of the 4 points above.

tejohnson added inline comments.Apr 21 2016, 5:37 AM
test/ThinLTO/X86/alias_resolution.ll
39

I don't remember the history unfortunately.

Just looked this up to refresh my memory. This was in the first version of D18346, and here is my comment as to why it wasn't legal to do the part that was converting weakany/linkonceany to available_externally in the multiple definition case:
http://reviews.llvm.org/D18346?id=51265#inline-155935

But as I pointed out later, you can still do the linkonceany->weakany conversion:
http://reviews.llvm.org/D18346?id=51265#inline-156013

I believe that is what is needed here when a linkonceany is exported, for correctness.

What I am gonna do is moving forward with this patch, leaving the bug as-is, since it is already
existing I think.

I don't believe there is a bug in the current upstream head, just in the libLTO case once D19096 goes in and prevents linkonce from being force imported.

I'll implement in a new patch (or a few patches) the proper handling of the 4 points above.

AFAICT the only place we are getting things wrong is with linkonce(any), due to your first point. And that should be easy to fix here - just do the conversion to weakany when the linkonceany is exported. (Then the name should probably be changed from ResolveODR to ResolveWeakLinkOnce or something more broad like that.)

The last point can't be done via available_externally for linkonceany/weakany due to the issue I pointed out in the old patch in the first link above. It could be done via dropping the unselected defs (they can't be inlined anyway), but that is a compile time optimization, not a correctness issue.