[PATCH] D46326: ThinLTO+CFI: short-circuit direct calls to jump table entries

[Dmitry Mikulin via Phabricator via llvm-commits]

dmikulin marked 2 inline comments as done.
dmikulin added a comment.

Can you guys please give it a try with Chromium? Do you get any measurable performance improvements with this change?



================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:985
+  if (F->isDeclarationForLinker() && isDefinition) {
+    // Functions with default visibility may be overriden at run time,
+    // don't short curcuit them
----------------
pcc wrote:
> Can you check for `dso_local` instead of default visibility here? I think that should be exactly the property that you want to check for.
Thanks, that's better.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1009
     FDecl->setVisibility(Visibility);
+    Visibility = GlobalValue::HiddenVisibility;
 
----------------
pcc wrote:
> Is there a reason to set the visibility indirectly via this variable here?
I need to set visibility late, after replaceCfiUses(). If I do it sooner, the symbol becomes dso_local, and it breaks the logic in replaceCfiUses().


https://reviews.llvm.org/D46326