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

[tejohnson]

Summary

Clang does not add type metadata to available_externally vtables. When
choosing a summary to look at for virtual function definitions, make
sure we skip summaries for any available externally vtables as they will
not describe any virtual function functions, which are only summarized
in the presence of type metadata on the vtable def. Simply look for the
corresponding strong def's summary.

[evgeny777]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

845

The code below doesn't go well along with the comment. For example it allows having multiple linkonce_odr/weak_odr summaries and multiple ones with available_externally linkage.

848–849

Can we use single find_if instead of lines 856-871 for the sake of simplicity?

auto I = llvm::find_if(
          P.VTableVI.getSummaryList(),
          [&](const std::unique_ptr<GlobalValueSummary> &Summary) {
            return !GlobalValue::isAvailableExternallyLinkage(Summary->linkage());
          });

[tejohnson]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

845

That is the intent of both the code and the comment, which might not be written very clearly. The code previously allowed multiple weak/linkonce odr, and now should also allow for multiple available_externally. Hence the comment "all but one must be available_externally" - because we can have multiple available externally but only one external linkage. Does that clarify things, or do you have alternate wording in mind that could be clearer?

Now that I think about it, this code should probably also proactively guard against the type of situation encountered and handled in D28411 and D67322 of same-named locals in same-named files compiled without enough distinguishing path. In that case we should not assert and just be conservatively correct. The easiest way to do that is to simply return false from this routine if that situation is encountered. I will add that handling.

848–849

Good idea, will change.

[evgeny777]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

845

Is it allowed having weak_odr + linkonce_odr + available_externally in P.VTableVI.getSummaryList()? If yes (I think it should be) than all but one must be available_externally is misleading. If not the code itself should be changed.

[tejohnson]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

845

Yes, that is a valid combination. I have changed the comment and the code. I made the lookup of the first non-available externally into a loop so I can also check for the case of multiple locals vtables with the same guid owing to not enough distinguishing path and same source file name (handled conservatively by returning false), and added a test case.

848–849

Changed as described above - use a loop so I can also look for the clashing local guid case.

[tejohnson]

Update patch per comments.

[evgeny777]

LGTM

[aganea]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

874

Hi Theresa!
I'm getting a crash when building Clang with ThinLTO.
The issue seems that VS ends up pointing an AliasSummary * instead of a GlobalVarSummary *, because of the !GlobalValue::isAvailableExternallyLinkage() test above, which leads to a crash when dereferencing the first element of VS->vTableFuncs().begin(), because it doesn't point to the right thing.
I'm at checkout 1cc0ba4 (from yesterday)
Please see the full repro:

bug_thinlto.txt14 KBDownload

[tejohnson]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

874

Thanks for the report and repro. I will take a look today. Conceptually it seems like a simple fix of invoking getBaseObject() on the summary is needed at some point in this loop.

[tejohnson]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

874

Fix sent for review in D71040.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

bool DevirtModule::tryFindVirtualCallTargets(
   // Give up if we couldn't find any targets.
   return !TargetsForSlot.empty();
 }
 
 bool DevirtIndex::tryFindVirtualCallTargets(
     std::vector<ValueInfo> &TargetsForSlot, const TypeIdCompatibleVtableInfo TIdInfo,
     uint64_t ByteOffset) {
   for (const TypeIdOffsetVtableInfo P : TIdInfo) {
-    // VTable initializer should have only one summary, or all copies must be
-    // linkonce/weak ODR.
+    // Ensure that we have at most one external linkage vtable initializer.
     assert(P.VTableVI.getSummaryList().size() == 1 ||
-           llvm::all_of(
+           llvm::count_if(
                P.VTableVI.getSummaryList(),
                [&](const std::unique_ptr<GlobalValueSummary> &Summary) {
-                 return GlobalValue::isLinkOnceODRLinkage(Summary->linkage()) ||
-                        GlobalValue::isWeakODRLinkage(Summary->linkage());
-               }));
-    const auto *VS = cast<GlobalVarSummary>(P.VTableVI.getSummaryList()[0].get());
-    if (!P.VTableVI.getSummaryList()[0]->isLive())
+                 return GlobalValue::isExternalLinkage(Summary->linkage());
+               }) <= 1);
+    // Find the first non-available_externally linkage vtable initializer.
+    // We can have multiple available_externally, linkonce_odr and weak_odr
+    // vtable initializers, however we want to skip available_externally as they
+    // do not have type metadata attached, and therefore the summary will not
+    // contain any vtable functions.
+    //
+    // Also, handle the case of same-named local Vtables with the same path
+    // and therefore the same GUID. This can happen if there isn't enough
+    // distinguishing path when compiling the source file. In that case we
+    // conservatively return false early.
+    const GlobalVarSummary *VS = nullptr;
+    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());
+    }
+    if (!VS->isLive())
       continue;
     for (auto VTP : VS->vTableFuncs()) {
       if (VTP.VTableOffset != P.AddressPointOffset + ByteOffset)
         continue;
 
       TargetsForSlot.push_back(VTP.FuncVI);
     }
   }
 
   // Give up if we couldn't find any targets.

llvm/test/ThinLTO/X86/Inputs/devirt_available_externally.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 (...)** }
+
+@_ZTV1D = constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.D*, i32)* @_ZN1D1mEi to i8*)] }, !type !3
+
+define i32 @_ZN1D1mEi(%struct.D* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+attributes #0 = { noinline optnone }
+
+!3 = !{i64 16, !"_ZTS1D"}

llvm/test/ThinLTO/X86/Inputs/devirt_local_same_guid.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"
+
+source_filename = "-"
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { %struct.A }
+
+@_ZTV1B = internal constant { [4 x i8*] } { [4 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.B*, i32)* @_ZN1B1fEi to i8*), i8* bitcast (i32 (%struct.A*, i32)* @_ZN1A1nEi to i8*)] }, !type !0, !type !1
+
+define internal i32 @_ZN1B1fEi(%struct.B* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+define internal i32 @_ZN1A1nEi(%struct.A* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+define i32 @test2(%struct.B* %obj, i32 %a) {
+entry:
+  %0 = bitcast %struct.B* %obj to i8***
+  %vtable2 = load i8**, i8*** %0
+  %1 = bitcast i8** %vtable2 to i8*
+  %p2 = call i1 @llvm.type.test(i8* %1, metadata !"_ZTS1B")
+  call void @llvm.assume(i1 %p2)
+
+  %fptrptr = getelementptr i8*, i8** %vtable2, i32 1
+  %2 = bitcast i8** %fptrptr to i32 (%struct.B*, i32)**
+  %fptr33 = load i32 (%struct.B*, i32)*, i32 (%struct.B*, i32)** %2, align 8
+
+  %call4 = tail call i32 %fptr33(%struct.B* nonnull %obj, i32 %a)
+  ret i32 %call4
+}
+
+attributes #0 = { noinline optnone }
+
+declare i1 @llvm.type.test(i8*, metadata)
+declare void @llvm.assume(i1)
+
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTS1B"}

llvm/test/ThinLTO/X86/devirt_available_externally.ll

+; REQUIRES: x86-registered-target
+
+; Test index-based devirtualization when first copy is available_externally,
+; which doesn't have type metadata. We should use the strong external
+; def in the other module to devirtualize.
+
+; 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_available_externally.ll
+
+; The available_externally copy should not get vTableFuncs information in its
+; summary entry, but the external def should.
+; RUN: llvm-dis -o - %t3.o | FileCheck %s --check-prefix=AVAILEXTERNAL
+; AVAILEXTERNAL: gv: (name: "_ZTV1D"
+; AVAILEXTERNAL-NOT: vTableFuncs
+; AVAILEXTERNAL-SAME: ; guid =
+; RUN: llvm-dis -o - %t4.o | FileCheck %s --check-prefix=EXTERNAL
+; EXTERNAL: gv: (name: "_ZTV1D", {{.*}} vTableFuncs: ((virtFunc:
+
+; 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:   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
+; RUN: llvm-nm %t5.1 | FileCheck %s --check-prefix=NM-INDEX1
+; RUN: llvm-nm %t5.2 | FileCheck %s --check-prefix=NM-INDEX2
+
+; NM-INDEX1-DAG: U _ZN1D1mEi
+
+; NM-INDEX2-DAG: T _ZN1D1mEi
+
+; 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 = available_externally constant { [3 x i8*] } { [3 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.D*, i32)* @_ZN1D1mEi to i8*)] }
+
+; 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 }

llvm/test/ThinLTO/X86/devirt_local_same_guid.ll

+; REQUIRES: x86-registered-target
+
+; Test that index-based devirtualization in the presence of same-named
+; local vtables in same named source files fails.
+
+; 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_local_same_guid.ll
+
+; RUN: llvm-lto2 run %t3.o %t4.o -save-temps -use-new-pm -pass-remarks=. \
+; RUN:   -wholeprogramdevirt-print-index-based \
+; RUN:   -o %t5 \
+; RUN:   -r=%t3.o,use_B,px \
+; RUN:   -r=%t3.o,test,px \
+; RUN:   -r=%t4.o,test2,px
+; RUN: llvm-dis %t5.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR1
+; RUN: llvm-dis %t5.2.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR2
+
+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"
+
+source_filename = "-"
+
+%struct.A = type { i32 (...)** }
+%struct.B = type { %struct.A }
+
+@_ZTV1B = internal constant { [4 x i8*] } { [4 x i8*] [i8* null, i8* undef, i8* bitcast (i32 (%struct.B*, i32)* @_ZN1B1fEi to i8*), i8* bitcast (i32 (%struct.B*, i32)* @_ZN1B1nEi to i8*)] }, !type !0, !type !1
+
+define internal i32 @_ZN1B1fEi(%struct.B* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+define internal i32 @_ZN1B1nEi(%struct.B* %this, i32 %a) #0 {
+   ret i32 0;
+}
+
+; Ensures that vtable of B is live so that we will attempt devirt.
+define dso_local i32 @use_B(%struct.B* %a) {
+entry:
+  %0 = bitcast %struct.B* %a to i32 (...)***
+  store i32 (...)** bitcast (i8** getelementptr inbounds ({ [4 x i8*] }, { [4 x i8*] }* @_ZTV1B, i64 0, inrange i32 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
+  ret i32 0
+}
+
+; CHECK-IR1: define i32 @test(
+define i32 @test(%struct.A* %obj, i32 %a) {
+entry:
+  %0 = bitcast %struct.A* %obj to i8***
+  %vtable = load i8**, i8*** %0
+  %1 = bitcast i8** %vtable to i8*
+  %p = call i1 @llvm.type.test(i8* %1, metadata !"_ZTS1A")
+  call void @llvm.assume(i1 %p)
+  %fptrptr = getelementptr i8*, i8** %vtable, i32 1
+  %2 = bitcast i8** %fptrptr to i32 (%struct.A*, i32)**
+  %fptr1 = load i32 (%struct.A*, i32)*, i32 (%struct.A*, i32)** %2, align 8
+
+  ; Check that the call was not devirtualized.
+  ; CHECK-IR1: %call = tail call i32 %fptr1
+  %call = tail call i32 %fptr1(%struct.A* nonnull %obj, i32 %a)
+
+  ret i32 %call
+}
+
+; CHECK-IR2: define i32 @test2
+; Check that the call was not devirtualized.
+; CHECK-IR2:   %call4 = tail call i32 %fptr
+
+declare i1 @llvm.type.test(i8*, metadata)
+declare void @llvm.assume(i1)
+
+attributes #0 = { noinline optnone }
+
+!0 = !{i64 16, !"_ZTS1A"}
+!1 = !{i64 16, !"_ZTS1B"}