[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

846

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.

856

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

846

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.

856

Good idea, will change.

[evgeny777]

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

846

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

846

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.

856

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(
   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.
+    // linkonce/weak ODR, or all but one must be available_externally.
     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());
-               }));
+                 return !(
+                     GlobalValue::isLinkOnceODRLinkage(Summary->linkage()) ||
+                     GlobalValue::isWeakODRLinkage(Summary->linkage()) ||
+                     GlobalValue::isAvailableExternallyLinkage(
+                         Summary->linkage()));
+               }) <= 1);
     const auto *VS = cast<GlobalVarSummary>(P.VTableVI.getSummaryList()[0].get());
-    if (!P.VTableVI.getSummaryList()[0]->isLive())
+    // If there is more than one summary, and the first isn't some kind of weak
+    // ODR linkage, then look for the first strong def (i.e. ignoring any
+    // available externally).
+    if (P.VTableVI.getSummaryList().size() > 1 &&
+        !GlobalValue::isLinkOnceODRLinkage(VS->linkage()) &&
+        !GlobalValue::isWeakODRLinkage(VS->linkage())) {
+      auto I = llvm::find_if(
+          P.VTableVI.getSummaryList(),
+          [&](const std::unique_ptr<GlobalValueSummary> &Summary) {
+            return GlobalValue::isExternalLinkage(Summary->linkage()) ||
+                   GlobalValue::isLocalLinkage(Summary->linkage());
+          });
+      assert(I != P.VTableVI.getSummaryList().end());
+      VS = cast<GlobalVarSummary>(I->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/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 }