[PATCH] D152741: [WPD] implement -funknown-vtable-visibility-filepaths

[Di Mo via Phabricator via cfe-commits]

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