Skip to content

Conversation

XChy
Copy link
Member

@XChy XChy commented Aug 28, 2025

As reported in #154668 (comment), we missed invalidating analysis as we don't set the MadeChanges to true after removing dead functions.

This patch makes it explicit to remove the dead functions marked by FuncSpec in SCCP and set MadeChanges correctly.

@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2025

@llvm/pr-subscribers-function-specialization

@llvm/pr-subscribers-llvm-transforms

Author: XChy (XChy)

Changes

As reported in #154668 (comment), we missed invalidating analysis as we don't set the MadeChanges to true after removing dead functions.

This patch makes it explicit to remove the dead functions marked by FuncSpec in SCCP and set MadeChanges correctly.


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

3 Files Affected:

  • (modified) llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h (+4-2)
  • (modified) llvm/lib/Transforms/IPO/FunctionSpecialization.cpp (+5-4)
  • (modified) llvm/lib/Transforms/IPO/SCCP.cpp (+3)
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h b/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
index 5a682e8c7b5eb..33ff917aa1540 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
@@ -261,10 +261,12 @@ class FunctionSpecializer {
       : Solver(Solver), M(M), FAM(FAM), GetBFI(GetBFI), GetTLI(GetTLI),
         GetTTI(GetTTI), GetAC(GetAC) {}
 
-  LLVM_ABI ~FunctionSpecializer();
-
   LLVM_ABI bool run();
 
+  // Clean dead functions and ssa copies introduced in specializations.
+  // Return true if removing any dead function.
+  bool cleanUp();
+
   InstCostVisitor getInstCostVisitorFor(Function *F) {
     auto &TTI = GetTTI(*F);
     return InstCostVisitor(GetBFI, F, M.getDataLayout(), TTI, Solver);
diff --git a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
index a459a9eddbcfc..01515c2b035b8 100644
--- a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
@@ -639,14 +639,15 @@ template <> struct llvm::DenseMapInfo<SpecSig> {
   }
 };
 
-FunctionSpecializer::~FunctionSpecializer() {
-  LLVM_DEBUG(
-    if (NumSpecsCreated > 0)
-      dbgs() << "FnSpecialization: Created " << NumSpecsCreated
+bool FunctionSpecializer::cleanUp() {
+  bool MadeChanges = !DeadFunctions.empty();
+  LLVM_DEBUG(if (NumSpecsCreated > 0) dbgs()
+             << "FnSpecialization: Created " << NumSpecsCreated
              << " specializations in module " << M.getName() << "\n");
   // Eliminate dead code.
   removeDeadFunctions();
   cleanUpSSA();
+  return MadeChanges;
 }
 
 /// Get the unsigned Value of given Cost object. Assumes the Cost is always
diff --git a/llvm/lib/Transforms/IPO/SCCP.cpp b/llvm/lib/Transforms/IPO/SCCP.cpp
index e98a70f228ada..25b001eb4aa3f 100644
--- a/llvm/lib/Transforms/IPO/SCCP.cpp
+++ b/llvm/lib/Transforms/IPO/SCCP.cpp
@@ -354,6 +354,9 @@ static bool runIPSCCP(
     ++NumGlobalConst;
   }
 
+  if (IsFuncSpecEnabled)
+    MadeChanges |= Specializer.cleanUp();
+
   return MadeChanges;
 }
 

@labrinea
Copy link
Collaborator

Why not set MadeChanges here https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/SCCP.cpp#L174 and leave everything else as is?

@XChy
Copy link
Member Author

XChy commented Aug 28, 2025

Why not set MadeChanges here https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/SCCP.cpp#L174 and leave everything else as is?

I prefer the current form just for direct logic, and for other similar changes in funcspec in future. I can set it as you suggest if you insist, thanks.

@antoniofrighetto
Copy link
Contributor

Why not set MadeChanges here https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/SCCP.cpp#L174 and leave everything else as is?

I prefer the current form just for direct logic, and for other similar changes in funcspec in future. I can set it as you suggest if you insist, thanks.

I'd also rather keep the destructor, if possible.

@XChy XChy force-pushed the fix-fnspecializer-invalidate-analysis branch from b56a528 to 9386571 Compare August 28, 2025 16:41
@XChy XChy enabled auto-merge (squash) August 28, 2025 16:52
@XChy XChy merged commit db86e3c into llvm:main Aug 28, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants