[ThinLTO/WPD] Fix index-based WPD for alias vtables

[tejohnson]

Summary

A recent fix in D69452 fixed index based WPD in the presence of
available_externally vtables. It added a cast of the vtable def
summary to a GlobalVarSummary. However, in some cases one def may be an
alias, in which case we need to get the base object before casting,
otherwise we will crash.

[aganea]

I was able to successfully create an ThinLTO-built clang.exe with this patch. However when compiling with assertions on, I'm still getting this:

Assertion failed: P.VTableVI.getSummaryList().size() == 1 || llvm::count_if( P.VTableVI.getSummaryList(), [&](const std::unique_ptr<GlobalValueSummary> &Summary) { return GlobalValue::isExternalLinkage(Summary->linkage()); }) <= 1, file D:\llvm-project\llvm\lib\Transforms\IPO\WholeProgramDevirt.cpp, line 852

I simply built a 2-stage clang with the following cmd-line:

stage 1: cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_LINKER="C:/Program Files/LLVM/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true -DLLVM_ENABLE_PDB=ON

stage 2: cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER="../stage1/bin/clang-cl.exe" -DCMAKE_CXX_COMPILER="../stage1/bin/clang-cl.exe" -DCMAKE_LINKER="../stage1/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true -DLLVM_ENABLE_PDB=ON -DLLVM_USE_CRT_RELEASE=MT -DCMAKE_CXX_FLAGS="-march=skylake-avx512 /GS- -Xclang -O3 -Xclang -fwhole-program-vtables -fstrict-aliasing" -DCMAKE_C_FLAGS="-march=skylake-avx512 /GS- -Xclang -O3 -Xclang -fwhole-program-vtables -fstrict-aliasing" -DLLVM_ENABLE_LTO=THIN -DLLVM_PARALLEL_LINK_JOBS=10

Let me know if you need more info.

[evgeny777]

Strange this assertion probably means, that there are two summaries with external linkage in VTableVI. I tried reproducer out of curiosity, but it worked fine for me (no assertion on stage2 with ninja clang).

[aganea]

In D71040#1783934, @evgeny777 wrote:

Strange this assertion probably means, that there are two summaries with external linkage in VTableVI. I tried reproducer out of curiosity, but it worked fine for me (no assertion on stage2 with ninja clang).

Could you possibly try check-all or check-llvm? This problem was occuring in the LLVM unit tests.

[evgeny777]

Could you possibly try check-all or check-llvm? This problem was occuring in the LLVM unit tests.

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

[tejohnson]

In D71040#1787416, @evgeny777 wrote:

Could you possibly try check-all or check-llvm? This problem was occuring in the LLVM unit tests.

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

This appears to be an unrelated issue then, and should probably be fixed in a separate patch. Can this one be approved and go in, and I'll make sure I can repro and then fix this one?

[aganea]

I've used this patch since you've published it, on different contexts, things look good from my user perspective.

llvm/test/ThinLTO/X86/Inputs/devirt_alias.ll

2

Do you think you can add a repro for creating this input file? It's not always trivial if changes are needed further down the line.

llvm/test/ThinLTO/X86/devirt_alias.ll

26

Same comment as for the other file above.

[aganea]

Ping @evgeny777 @steven_wu!
Do you think you can take a look at this please? It'd be nice if it could land before the 10.0 branch. Thanks!

[evgeny777]

LGTM

[tejohnson]

llvm/test/ThinLTO/X86/Inputs/devirt_alias.ll

2

I'm not really sure what to put, as it has been awhile but it looks like I cloned a different devirt test and modified it to fit the situation that triggered this bug.

[tejohnson]

In D71040#1808470, @tejohnson wrote:

In D71040#1787416, @evgeny777 wrote:

Could you possibly try check-all or check-llvm? This problem was occuring in the LLVM unit tests.

This did work. We have two vftable aliases with external linkage, that's why assertion fires. They're both attached to comdat, so there are no link errors.
May be just remove assertion?

This appears to be an unrelated issue then, and should probably be fixed in a separate patch. Can this one be approved and go in, and I'll make sure I can repro and then fix this one?

Fix for assertion (remove it and add test) mailed in D72648.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

for (const TypeIdOffsetVtableInfo &P : TIdInfo) {
     bool LocalFound = false;
     for (auto &S : P.VTableVI.getSummaryList()) {
       if (GlobalValue::isLocalLinkage(S->linkage())) {
         if (LocalFound)
           return false;
         LocalFound = true;
       }
       if (!GlobalValue::isAvailableExternallyLinkage(S->linkage()))
-        VS = cast<GlobalVarSummary>(S.get());
+        VS = cast<GlobalVarSummary>(S->getBaseObject());
     }
     if (!VS->isLive())
       continue;
     for (auto VTP : VS->vTableFuncs()) {
       if (VTP.VTableOffset != P.AddressPointOffset + ByteOffset)
         continue;
 
       TargetsForSlot.push_back(VTP.FuncVI);

llvm/test/ThinLTO/X86/Inputs/devirt_alias.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+%struct.D = type { i32 (...)** }
+
+@some_name = constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.D*, i32)* @_ZN1D1mEi to i8*)] }, !type !3
+@_ZTV1D = alias { [3 x i8*] }, { [3 x i8*] }* @some_name
+
+define i32 @_ZN1D1mEi(%struct.D* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+attributes #0 = { noinline optnone }
+
+!3 = !{i64 16, !"_ZTS1D"}

llvm/test/ThinLTO/X86/devirt_alias.ll

+; REQUIRES: x86-registered-target
+
+; Test index-based devirtualization when one copy is an alias.
+
+; Generate unsplit module with summary for ThinLTO index-based WPD.
+; RUN: opt -thinlto-bc -o %t3.o %s
+; RUN: opt -thinlto-bc -o %t4.o %p/Inputs/devirt_alias.ll
+
+; RUN: llvm-lto2 run %t3.o %t4.o -save-temps -pass-remarks=. \
+; RUN:   -wholeprogramdevirt-print-index-based \
+; RUN:   -o %t5 \
+; RUN:   -r=%t3.o,test,px \
+; RUN:   -r=%t3.o,_ZTV1D, \
+; RUN:   -r=%t3.o,_ZN1D1mEi, \
+; RUN:   -r=%t4.o,_ZN1D1mEi,p \
+; RUN:   -r=%t4.o,_ZTV1D,px \
+; RUN:   -r=%t4.o,some_name,px \
+; RUN:   2>&1 | FileCheck %s --check-prefix=REMARK --check-prefix=PRINT
+; RUN: llvm-dis %t5.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR1
+
+; PRINT-DAG: Devirtualized call to {{.*}} (_ZN1D1mEi)
+
+; REMARK-DAG: single-impl: devirtualized a call to _ZN1D1mEi
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+%struct.D = type { i32 (...)** }
+
+@_ZTV1D = linkonce_odr constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.D*, i32)* @_ZN1D1mEi to i8*)] }, !type !3
+
+; CHECK-IR1-LABEL: define i32 @test
+define i32 @test(%struct.D* %obj2, i32 %a) {
+entry:
+  %0 = bitcast %struct.D* %obj2 to i8***
+  %vtable2 = load i8**, i8*** %0
+  %1 = bitcast i8** %vtable2 to i8*
+  %p2 = call i1 @llvm.type.test(i8* %1, metadata !"_ZTS1D")
+  call void @llvm.assume(i1 %p2)
+
+  %2 = bitcast i8** %vtable2 to i32 (%struct.D*, i32)**
+  %fptr33 = load i32 (%struct.D*, i32)*, i32 (%struct.D*, i32)** %2, align 8
+
+  ; Check that the call was devirtualized.
+  ; CHECK-IR1: %call4 = tail call i32 @_ZN1D1mEi
+  %call4 = tail call i32 %fptr33(%struct.D* nonnull %obj2, i32 %a)
+  ret i32 %call4
+}
+; CHECK-IR1-LABEL: ret i32
+; CHECK-IR1-LABEL: }
+
+declare i1 @llvm.type.test(i8*, metadata)
+declare void @llvm.assume(i1)
+declare i32 @_ZN1D1mEi(%struct.D* %this, i32 %a)
+
+attributes #0 = { noinline optnone }
+
+!3 = !{i64 16, !"_ZTS1D"}