[PATCH] D152406: [ThinLTO] Ignore callee edge to global variable

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson created this revision.
tejohnson added a reviewer: mtrofin.
Herald added subscribers: wlei, ormris, wenlei, hiraditya, inglorion.
Herald added a project: All.
tejohnson requested review of this revision.
Herald added a project: LLVM.

Since the symbols in the ThinLTO summary are indexed by GUID we can end
up in corner cases where a callee edge in the combined index goes to a
summary for a global variable. This could happen in the case of hash
collisions, and in the case of SamplePGO profiles could potentially happen
due to code changes (since we synthesize call edges to GUIDs that were
inlined callees in the profiled code).

Handle this by simply ignoring any non-FunctionSummary callees.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152406

Files:
  llvm/lib/Transforms/IPO/FunctionImport.cpp
  llvm/test/Transforms/FunctionImport/Inputs/funcimport_var2.ll
  llvm/test/Transforms/FunctionImport/funcimport_var.ll


Index: llvm/test/Transforms/FunctionImport/funcimport_var.ll
===================================================================
--- llvm/test/Transforms/FunctionImport/funcimport_var.ll
+++ llvm/test/Transforms/FunctionImport/funcimport_var.ll
@@ -1,16 +1,20 @@
-; This test makes sure a static var is not selected as a callee target
-; (which will crash compilation).
+;; This test makes sure a static var is not selected as a callee target (it will
+;; have a different GUID). Similarly, ensure that a callee GUID annotated from a
+;; sample pgo profile that matches a global variable GUID (either due to code
+;; changes or a hash collision), does not get considered for importing. Either
+;; of these can crash compilation.
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport_var2.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=thinlink -o %t3 %t.bc %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=import -thinlto-index=%t3 %t.bc %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=run %t.bc %t2.bc -exported-symbol=_Z4LinkPKcS0_
-; RUN: llvm-nm %t.bc.thinlto.o | FileCheck %s
+; RUN: llvm-nm %t.bc.thinlto.o | FileCheck %s --implicit-check-not=globalvar
 ; RUN: llvm-lto2 run %t.bc %t2.bc -o %t.out \
 ; RUN:   -r %t.bc,_Z4LinkPKcS0_,plx \
 ; RUN:   -r %t.bc,link,l \
+; RUN:   -r %t2.bc,globalvar,plx \
 ; RUN:   -r %t2.bc,get_link,plx
-; RUN: llvm-nm %t.out.1 | FileCheck %s
+; RUN: llvm-nm %t.out.1 | FileCheck %s --implicit-check-not=globalvar
 ; CHECK: U link
 
 ; REQUIRES: x86-registered-target
@@ -18,10 +22,13 @@
 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-unknown-linux-gnu"
 
-define i32 @_Z4LinkPKcS0_(ptr, ptr) local_unnamed_addr  {
+define i32 @_Z4LinkPKcS0_(ptr, ptr) local_unnamed_addr !prof !1 {
   %3 = tail call i32 @link(ptr %0, ptr %1) #2
   ret i32 %3
 }
 
 ; Function Attrs: nounwind
-declare i32 @link(ptr, ptr) local_unnamed_addr 
+declare i32 @link(ptr, ptr) local_unnamed_addr
+
+;; This matches the GUID of global variable @globalvar in the other input.
+!1 = !{!"function_entry_count", i64 110, i64 12887606300320728018}
Index: llvm/test/Transforms/FunctionImport/Inputs/funcimport_var2.ll
===================================================================
--- llvm/test/Transforms/FunctionImport/Inputs/funcimport_var2.ll
+++ llvm/test/Transforms/FunctionImport/Inputs/funcimport_var2.ll
@@ -3,6 +3,9 @@
 
 @link = internal global i32 0, align 4
 
+; guid = 12887606300320728018
+ at globalvar = global i32 0, align 4
+
 ; Function Attrs: norecurse nounwind readnone uwtable
 define nonnull ptr @get_link() local_unnamed_addr {
   ret ptr @link
Index: llvm/lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -179,7 +179,15 @@
           return {FunctionImporter::ImportFailureReason::InterposableLinkage,
                   GVSummary};
 
-        auto *Summary = cast<FunctionSummary>(GVSummary->getBaseObject());
+        auto *Summary = dyn_cast<FunctionSummary>(GVSummary->getBaseObject());
+
+        // Ignore any callees that aren't actually functions. This could happen
+        // in the case of GUID hash collisions. It could also happen in theory
+        // for SamplePGO profiles collected on old versions of the code after
+        // renaming, since we synthesize edges to any inlined callees appearing
+        // in the profile.
+        if (!Summary)
+          return {FunctionImporter::ImportFailureReason::GlobalVar, GVSummary};
 
         // If this is a local function, make sure we import the copy
         // in the caller's module. The only time a local function can


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D152406.529460.patch
Type: text/x-patch
Size: 3796 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230607/423f09ff/attachment.bin>