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

[Wenlei He via Phabricator via cfe-commits]

wenlei added a comment.

> The big advantage of doing this in the FE is that we know which types are actually coming from the native headers. Blocking all types in the TU is overly conservative and also less stable as header changes can effectively turn on/off unrelated large chunks of WPD.

This is clearly going to be selective in punting unsafe devirt, however do you have data comparing the effectiveness of the two (module granularity vs header granularity)?

I also think introducing unknown visibility is a good idea for this to work, but this is going to be exposed to users (not hidden implementing only), so we would probably need to have spec/rule to handle conflicting visibility from different source and make those explicit here: https://clang.llvm.org/docs/LTOVisibility.html.

There's a spectrum of solutions we could use to make WPD safer, but we need to be careful not to make this whole thing too convoluted with multiple solutions implemented, but little differentiation in their incremental value (extra perf, extra safety). So having concrete data backing the incremental value of this solution would be helpful.



================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:191
+  struct RegexWithPattern {
+    std::string Pattern;
+    std::shared_ptr<llvm::Regex> Regex;
----------------
Pattern string doesn't seem to be used anywhere, can we simplify this using `llvm::Regex` instead of `RegexWithPattern`?


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:198
+
+    /// Returns true iff the optimization remark holds a valid regular
+    /// expression.
----------------
update comment


================
Comment at: clang/include/clang/Driver/Options.td:3160
   NegFlag<SetFalse>, BothFlags<[CoreOption]>>;
+def funknown_vtable_visibility_filepaths_EQ : Joined<["-"], "funknown-vtable-visibility-filepaths=">, Group<f_Group>,
+  HelpText<"Skip types from participating in whole-program vtable optimization defined under these filepaths. Pass empty string to unset">,
----------------
nit: `filepaths` -> `filepath` - this does not accept a comma separated list of paths(regexs).


================
Comment at: clang/include/clang/Driver/Options.td:3161
+def funknown_vtable_visibility_filepaths_EQ : Joined<["-"], "funknown-vtable-visibility-filepaths=">, Group<f_Group>,
+  HelpText<"Skip types from participating in whole-program vtable optimization defined under these filepaths. Pass empty string to unset">,
+  Flags<[CC1Option]>;
----------------
> Pass empty string to unset 

Remove this to be concise?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2001
+    if (!Pattern->isValid(RegexError)) {
+      Diags.Report(diag::err_drv_optimization_remark_pattern)
+          << RegexError << A->getAsString(Args);
----------------
rename `err_drv_optimization_remark_pattern` so it's not specific to remarks. 


================
Comment at: llvm/include/llvm/IR/GlobalObject.h:41
+    // Type is unknown and should always be treated conservatively.
+    VCallVisibilityUnknown = 3,
   };
----------------
This probably needs doc change too: https://clang.llvm.org/docs/LTOVisibility.html.

We also need to define when flag derived visibility conflicts with attribute derived visibility, what is the priority order. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152741