modimo added a comment.
In D152741#4445807 <https://reviews.llvm.org/D152741#4445807>, @wenlei wrote:
>> For concrete data are you talking about between the different solutions e.g.
>> --lto-whole-program-visibility, -funknown-vtable-visibility-filepaths, RTTI
>> based, FatLTO based etc or something else?
>
> Right, between the different solutions. RTTI based solution doesn't exist
> yet, so maybe just compare using `-fwhole-program-vtables` on a known safe
> set of files vs using `-funknown-vtable-visibility-filepaths` on a known
> unsafe set of files first.
On a large Meta service with manually opting in an internal source folder with
`-fwhole-program-vtables` there's 2,933 single implementation methods that get
devirtualized. Using `-funknown-vtable-visibility-filepath` on the same service
for the `third-party` directory there's 32,800 single implementation method
devirts.
>> The ordering for conflicts is embedded in the logic for
>> CodeGenModule::GetVCallVisibilityLevel which has priority order of
>
> I was thinking about different source of visibility instead of absolute order
> of visibility itself - i.e. what is the rule if
> `__attribute__((visibility("...")))` conflicts with
> `-funknown-vtable-visibility-filepaths` setting for a specific type? This may
> not be an immediately important question, but just as example of the knock on
> effect of added complexity, which may or may not be justified depending on
> the benefit, which goes back to data from experiments.
That complexity already exists with `-fvisibility=hidden` interacting with
`__attribute__((visibility("...")))` where the most conservative annotation
wins out. Having a type annotated with `unknown` visibility is just adding a
more conservative option than `public`.
> We have `-wholeprogramdevirt-skip`; with this patch, we will have
> `-funknown-vtable-visibility-filepaths`; later on, we will have another RTTI
> based solution, then there's FatObj solution. It feels like a lot of stuff
> trying to solve one problem, so wondering if this addition here is going to
> provide enough value in the end state.
My current prototype RTTI implementation doesn't really need an `unknown`
visibility because it's generating and passing a blocklist at symbol resolution
time. For FatObj, the input into WPD is identical to when everything is built
with ThinLTO so `unknown` isn't that valuable either. The original intent was
to use this to roll out WPD to select services but performance-wise opting in
folders with `-fwhole-program-vtables` proves just as effective without having
to modify LLVM. With that use case gone, there's no longer a need on my side
for this change. Others may find value for this in the interim to
on-board/evaluate WPD but that's not very concrete value.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152741/new/
https://reviews.llvm.org/D152741
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
