[PATCH] D96721: [WPD][lld] Test handling of vtable definition from shared libraries

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 4 inline comments as done.
tejohnson added a comment.

I've addressed all the test suggestions, reply to the source code question below.

In D96721#2564767 <https://reviews.llvm.org/D96721#2564767>, @MaskRay wrote:

> (I did not know much about WholeProgramDevirt but I did spend some time to get an idea what the pass does.)

Thanks!

> Just adding the following code
>
>   if (!VS)
>         return false;
>
> without introducing `CurVS` can make the new test pass as well. Is the `CurVS` change needed?

Probably, but comment further below about why I think this change is still good to have.

> Also, regarding `if (!VS) return false;`, can `TargetsForSlot.push_back(VTP.FuncVI);` add an element in a previous iteration before `if (!VS) return false;` triggers?

It could, for another vtable compatible with the current type id, but it will be ignored. The caller doesn't do anything with TargetsForSlot if this method returns false.



================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_shared_def.ll:17
+;; Generate split module with summary for hybrid Thin/Regular LTO WPD.
+; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o %t1.o %s
+; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o %t2.o %S/Inputs/devirt_vcall_vis_shared_def.ll
----------------
MaskRay wrote:
> Nit: not reusing the same output filename for different opt commands may make debugging easier.
> 
> While debugging this to understand what happens, I need to run a specific ld.lld command and have to confirm the %t1.o object file is produced by the desired opt command...
Modified everything to be unique.


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_shared_def.ll:34
+;; containing the strong defs of the vtables.
+; RUN: opt -o %t5.o %S/Inputs/devirt_vcall_vis_shared_def.ll
+; RUN: ld.lld %t5.o -o %t5.so -shared
----------------
MaskRay wrote:
> %t5.o is the same as %t2.o
Yep, and with the unique names used now I can also avoid re-doing the rest of the opt invocations below.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1024
-      }
+      } else
+        // Currently clang will not attach the necessary type metadata to
+        // available_externally vtables.
+        assert(GlobalValue::isAvailableExternallyLinkage(S->linkage()));
     }
     if (!VS->isLive())
----------------
MaskRay wrote:
> (I did not have WholeProgramDevirt but I did spend some time to get an idea what the pass does.)
> 
> Just adding the following code
> ```
> if (!VS)
>       return false;
> ```
> 
> without introducing `CurVS` can make the new test pass as well. Is the `CurVS` change needed?
For correctness reasons, I think you are right in that we could probably just ignore all available_externally vtables associated with the current type id - if we only have available_externally copies of a particular vtable then there's no way we should promote. But after thinking through this some more, it makes sense to use the new check I've added there for compile time reasons - if we have many copies of an available_externally vtable (possible since this may come from a header that is widely included), if the symbol is defined in a library and was thus marked ExternDynamic, with the new code we don't waste time walking through all copies but will return false early on the first copy (which will have public visibility).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96721