[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

It doesn't seem sound to create the distinction between translation unit and linkage unit visibility and then allow passes to relax visibility. For example, imagine that you have an executable with (compiled with `-fvisibility=default`):

  struct PluginInterface {
    virtual void foo() = 0;
  }
  
  struct Plugin1 : PluginInterface {
    virtual void foo();
  };
  
  void Plugin1::foo() {
   // ...
  }

And a DSO loaded with dlopen:

  void call_foo(PluginInterface *plugin) {
    plugin->foo();
  }

Note that the lack of attribute means that the vtables have public LTO visibility. Depending on the contents of the DSO, the `PluginInterface` and `Plugin1` vtable symbols may not be referenced at all by the DSO. If the DSO is loaded via `dlopen` it will definitely not be referenced at link time. If we allow LTO to relax `PluginInterface` and `Plugin1` vtables to what you're calling `VCallVisibilityTranslationUnit` in the main executable and there are no calls to `foo()` in the main executable, we will incorrectly drop the definition of `Plugin1::foo()`.

The approach that I can see working involves basically the same structure as CFI and WPD: the LTO visibility is decided by the frontend and not relaxed by passes or LTO. There is only one "flavour" of `!vcall_visibility` metadata: the one that means the entire hierarchy below the vtable has hidden LTO visibility.

> However, the
>  changes in clang to use the llvm.type.checked.load intrinsic are causing ~1%
>  performance regression in the C++ parts of SPEC2006.

Have you checked the assembly to see whether an unused CFI check is being emitted?



================
Comment at: clang/lib/CodeGen/CGClass.cpp:2816
+  } else {
+    Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::assume), CheckResult);
+  }
----------------
There should be no need to emit this `llvm.assume` intrinsic. We can rely on the unspecified behaviour in the case where the second element returned by `llvm.type.checked.load` is false.


================
Comment at: llvm/docs/TypeMetadata.rst:240
+
+  __attribute__((visibility("public")))
+  struct A {
----------------
`s/"public"/"default"/`


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:183
+    // unit, we know that we can see all virtual functions which might use it,
+    // so VFE is safe.
+    if (auto GO = dyn_cast<GlobalObject>(&GV)) {
----------------
Not necessarily, at least in this implementation, because "vtable symbol can be internalized" doesn't imply "all vcalls are visible". See main response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932