[PATCH] D61255: [ThinLTO] Make weak data symbols prevailing when they're visible to regular object

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added a comment.

> I agree this doesn't look like the correct fix, especially the check !Sym.isExecutable() which doesn't look correct to me

The problem is that linkonce_odr functions and data should be IMO treated differently. The linkonce_odr linkage implies linker leaves only one copy of the symbol in the final image.
However it's not really a problem having multiple internal copies of linkonce_odr function because all copies follow one definition rule. This fact allows ThinLTO to efficiently optimize such function calls.
Situation however is different with linkonce_odr objects, because it's wrong to have multiple copies of linkonce_odr object because reads and writes supposed to access same memory will actually become
inconsistent. However there is still a nuance: if object is a compile time constant there can't be any write access to it, so we can safely internalize it.

The change is probably not correct anyway just because making prevailing something which isn't originally marked as prevailing seems like a bad idea.

> Maybe using ReadOnly bits in summary

`ReadOnly` can be used only after `computeDeadSymbols` is executed, so IMO not gonna work

>   or UnnamedAddr bits in resolution is a better approach 

How this can help?

> There is also a bit in GlobalResolution VisibleOutsideSummary which sounds like is designed to solve this problem?

Like I said, the problem, as I understand it, is that linkonce_odr function visible outside summary can be internalized, but non-constant object can't.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61255/new/

https://reviews.llvm.org/D61255