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

[Steven Wu via Phabricator via llvm-commits]

steven_wu added a comment.

In D61255#1484855 <https://reviews.llvm.org/D61255#1484855>, @evgeny777 wrote:

> > 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.


I think you are making the assumption of the input is C++ and using the one definition rule of C++ to guide the optimization which  I don't think it is appropriate here.
If there is some special semantics behind the symbol, it should be expressed in the IR. I could be missing something but I don't think LLVM IR has such semantics behind functions vs. global variables. Maybe you are trying to use function type to infer functions are constants?

> 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?

unnamed_addr means the address doesn't matter and it should be one of the important reason behind whether something can be internalized or not. If the address of a linkonce_odr function is taken and later used to compare in a different module, you can't internalize the function at all.
I think if it is unnamed_addr constants, it is always safe to internalize but I am not sure just the unnamed_adder is strong enough to indicate that or if we will miss too much assume everything if we skip everything doesn't have unnamed_addr because it might affect linker resolution.

>> 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