[ThinLTO]Mark referencers of local ifunc not eligible for import #92431

minglotus-6 · 2024-05-16T17:04:54Z

If an ifunc has local linkage, do not add it into ref edges and mark its referencer (a function or global variable) not eligible for import. An ifunc doesn't have summary and ThinLTO cannot promote it. Importing the referencer may cause linkage errors.

To reference a similar fix, https://reviews.llvm.org/D158961 marks callers of local ifunc not eligible for import to fix #58740

llvmbot · 2024-05-16T17:20:17Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-lto

Author: Mingming Liu (minglotus-6)

Changes

If an ifunc has local linkage, do not add it into ref edges and mark its referencer (a function or global variable) not eligible for import. An ifunc doesn't have summary and ThinLTO cannot promote it. Importing the referencer may cause linkage errors.

To reference a similar fix, https://reviews.llvm.org/D158961 marks callers of local ifunc not eligible for import to fix #58740

Full diff: https://github.com/llvm/llvm-project/pull/92431.diff

2 Files Affected:

(modified) llvm/lib/Analysis/ModuleSummaryAnalysis.cpp (+35-17)
(added) llvm/test/ThinLTO/X86/ref-ifunc.ll (+46)

diff --git a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
index c3d15afe6d9cb..36cff096f33aa 100644
--- a/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
+++ b/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
@@ -88,16 +88,22 @@ extern cl::opt<unsigned> MaxNumVTableAnnotations;
 // Walk through the operands of a given User via worklist iteration and populate
 // the set of GlobalValue references encountered. Invoked either on an
 // Instruction or a GlobalVariable (which walks its initializer).
-// Return true if any of the operands contains blockaddress. This is important
-// to know when computing summary for global var, because if global variable
-// references basic block address we can't import it separately from function
-// containing that basic block. For simplicity we currently don't import such
-// global vars at all. When importing function we aren't interested if any
-// instruction in it takes an address of any basic block, because instruction
-// can only take an address of basic block located in the same function.
+//
+// Return true if any user makes the analyzed instruction or global variable not
+// eligible to import.
+// - If a local-linkage ifunc is referenced, mark the analyzed value as not
+// eligible for import.
+// - Additionally, global vars are marked as not eligible to import if they
+// references references basic block address, because we can't import it
+// separately from function containing that basic block. For simplicity we
+// currently don't import such global vars at all. When importing functions we
+// aren't interested if any instruction in it takes an address of any basic
+// block, because instruction can only take an address of basic block located in
+// the same function.
 static bool findRefEdges(ModuleSummaryIndex &Index, const User *CurUser,
                          SetVector<ValueInfo, std::vector<ValueInfo>> &RefEdges,
-                         SmallPtrSet<const User *, 8> &Visited) {
+                         SmallPtrSet<const User *, 8> &Visited,
+                         bool &RefLocalLinkageIFunc) {
   bool HasBlockAddress = false;
   SmallVector<const User *, 32> Worklist;
   if (Visited.insert(CurUser).second)
@@ -119,8 +125,18 @@ static bool findRefEdges(ModuleSummaryIndex &Index, const User *CurUser,
         // We have a reference to a global value. This should be added to
         // the reference set unless it is a callee. Callees are handled
         // specially by WriteFunction and are added to a separate list.
-        if (!(CB && CB->isCallee(&OI)))
+        if (!(CB && CB->isCallee(&OI))) {
+          // If an ifunc has local linkage, do not add it into ref edges and
+          // mark its referencer not eligible for import. An ifunc doesn't have
+          // summary and ThinLTO cannot promote it; importing the referencer may
+          // cause linkage errors.
+          if (auto *GI = dyn_cast_if_present<GlobalIFunc>(GV);
+              GI && GI->hasLocalLinkage()) {
+            RefLocalLinkageIFunc = true;
+            continue;
+          }
           RefEdges.insert(Index.getOrInsertValueInfo(GV));
+        }
         continue;
       }
       if (Visited.insert(Operand).second)
@@ -313,7 +329,8 @@ static void computeFunctionSummary(
 
   // Add personality function, prefix data and prologue data to function's ref
   // list.
-  findRefEdges(Index, &F, RefEdges, Visited);
+  bool HasIFunc = false;
+  findRefEdges(Index, &F, RefEdges, Visited, HasIFunc);
   std::vector<const Instruction *> NonVolatileLoads;
   std::vector<const Instruction *> NonVolatileStores;
 
@@ -326,7 +343,7 @@ static void computeFunctionSummary(
 
   bool HasInlineAsmMaybeReferencingInternal = false;
   bool HasIndirBranchToBlockAddress = false;
-  bool HasIFuncCall = false;
+
   bool HasUnknownCall = false;
   bool MayThrow = false;
   for (const BasicBlock &BB : F) {
@@ -372,11 +389,11 @@ static void computeFunctionSummary(
             // of calling it we should add GV to RefEdges directly.
             RefEdges.insert(Index.getOrInsertValueInfo(GV));
           else if (auto *U = dyn_cast<User>(Stored))
-            findRefEdges(Index, U, RefEdges, Visited);
+            findRefEdges(Index, U, RefEdges, Visited, HasIFunc);
           continue;
         }
       }
-      findRefEdges(Index, &I, RefEdges, Visited);
+      findRefEdges(Index, &I, RefEdges, Visited, HasIFunc);
       const auto *CB = dyn_cast<CallBase>(&I);
       if (!CB) {
         if (I.mayThrow())
@@ -450,7 +467,7 @@ static void computeFunctionSummary(
         // Non-local ifunc is not cloned and does not have the issue.
         if (auto *GI = dyn_cast_if_present<GlobalIFunc>(CalledValue))
           if (GI->hasLocalLinkage())
-            HasIFuncCall = true;
+            HasIFunc = true;
         // Skip inline assembly calls.
         if (CI && CI->isInlineAsm())
           continue;
@@ -555,7 +572,7 @@ static void computeFunctionSummary(
                            SmallPtrSet<const User *, 8> &Cache) {
       for (const auto *I : Instrs) {
         Cache.erase(I);
-        findRefEdges(Index, I, Edges, Cache);
+        findRefEdges(Index, I, Edges, Cache, HasIFunc);
       }
     };
 
@@ -633,7 +650,7 @@ static void computeFunctionSummary(
   bool NonRenamableLocal = isNonRenamableLocal(F);
   bool NotEligibleForImport = NonRenamableLocal ||
                               HasInlineAsmMaybeReferencingInternal ||
-                              HasIndirBranchToBlockAddress || HasIFuncCall;
+                              HasIndirBranchToBlockAddress || HasIFunc;
   GlobalValueSummary::GVFlags Flags(
       F.getLinkage(), F.getVisibility(), NotEligibleForImport,
       /* Live = */ false, F.isDSOLocal(), F.canBeOmittedFromSymbolTable(),
@@ -787,7 +804,8 @@ static void computeVariableSummary(ModuleSummaryIndex &Index,
                                    SmallVectorImpl<MDNode *> &Types) {
   SetVector<ValueInfo, std::vector<ValueInfo>> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
-  bool HasBlockAddress = findRefEdges(Index, &V, RefEdges, Visited);
+  bool Unused = false;
+  bool HasBlockAddress = findRefEdges(Index, &V, RefEdges, Visited, Unused);
   bool NonRenamableLocal = isNonRenamableLocal(V);
   GlobalValueSummary::GVFlags Flags(
       V.getLinkage(), V.getVisibility(), NonRenamableLocal,
diff --git a/llvm/test/ThinLTO/X86/ref-ifunc.ll b/llvm/test/ThinLTO/X86/ref-ifunc.ll
new file mode 100644
index 0000000000000..714ae83cb1abb
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/ref-ifunc.ll
@@ -0,0 +1,46 @@
+; RUN: opt -module-summary %s -o %t.bc
+
+; RUN: llvm-dis %t.bc -o - | FileCheck %s
+
+; Tests that caller is not eligible to import and it doesn't have refs to ifunc 'callee'
+
+; CHECK: gv: (name: "caller", summaries: (function: ({{.*}}, flags: ({{.*}}notEligibleToImport: 1
+; CHECK-NOT: refs
+; CHECK-SAME: guid = 16677772384402303968
+
+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"
+
+@__cpu_model = external global { i32, i32, i32, [1 x i32] }
+
+@callee = internal ifunc void(), ptr @callee.resolver
+
+define void @dispatch(ptr %func) {
+    tail call void %func()
+    ret void
+}
+
+define void @caller() {
+  tail call void @dispatch(ptr @callee)
+  ret void
+}
+
+define internal ptr @callee.resolver() {
+resolver_entry:
+  tail call void @__cpu_indicator_init()
+  %0 = load i32, ptr getelementptr inbounds ({ i32, i32, i32, [1 x i32] }, ptr @__cpu_model, i64 0, i32 3, i64 0)
+  %1 = and i32 %0, 1024
+  %.not = icmp eq i32 %1, 0
+  %func_sel = select i1 %.not, ptr @callee.default.1, ptr @callee.avx2.0
+  ret ptr %func_sel
+}
+
+define internal void @callee.default.1(i32 %a) {
+  ret void
+}
+
+define internal void @callee.avx2.0(i32 %a) {
+  ret void
+}
+
+declare void @__cpu_indicator_init()

teresajohnson · 2024-05-16T17:27:08Z