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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

(I did not know much about 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?

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?



================
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
----------------
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...


================
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
----------------
%t5.o is the same as %t2.o


================
Comment at: lld/test/ELF/lto/devirt_vcall_vis_shared_def.ll:90
+define available_externally i32 @_ZN1A1fEi(%struct.A* %this, i32 %a) #0 {
+   ret i32 0;
+}
----------------
Drop `;`

ditto 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())
----------------
(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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96721