[FuncSpec] Return changed if function is changed by tryToReplaceWithConstant

[ChuanqiXu]

Summary

The may get changed before specialization by RunSCCPSolver. In other words, the pass may change the function without specialization happens. Add test and comment to reveal this.
And it may return No Changed if the function get changed by RunSCCPSolver before the specialization. It looks like a potential bug.

Test Plan: check-all

[SjoerdMeijer]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

765

I think this now deserves a comment.

[ChuanqiXu]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

765

hmm, I think it is easy to know that tryToReplaceWithConstant may change the function. I tried to add comments for this. But I find my words are a little redundant. I would make it if there is any good suggestion.

[SjoerdMeijer]

LGTM.

The remark about the comment was a nit anyway. Feel free to modify or omit it before committing.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

765

I thought the idea was that this:

// FIXME: The solver may make changes to the function here, so set Changed, even if later
// function specialization does not trigger.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

 //   a direct way to steer function specialization, avoiding the cost-model,
 //   and thus control compile-times / code-size.
 //
 // Todos:
 // - Limit the times a recursive function get specialized when
 // `func-specialization-max-iters`
 //   increases linearly. See discussion in https://reviews.llvm.org/D106426 for
 //   details.
+// - Don't transform the function if there is no function specialization
+// happens.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/CodeMetrics.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/InlineCost.h"
while (ResolvedUndefs) {
           ResolvedUndefs = true;
     }
 
     for (auto *F : WorkList) {
       for (BasicBlock &BB : *F) {
         if (!Solver.isBlockExecutable(&BB))
           continue;
         for (auto &I : make_early_inc_range(BB))
-          FS.tryToReplaceWithConstant(&I);
+          Changed |= FS.tryToReplaceWithConstant(&I);
       }
     }
   };
 
   auto &TrackedFuncs = Solver.getArgumentTrackedFunctions();
   SmallVector<Function *, 16> FuncDecls(TrackedFuncs.begin(),
                                         TrackedFuncs.end());
 #ifndef NDEBUG

llvm/test/Transforms/FunctionSpecialization/function-specialization-constant-expression.ll

+; Test function specialization wouldn't crash due to constant expression.
+; Note that this test case shows that function specialization pass would
+; transform the function even if no specialization happened.
+; RUN: opt -function-specialization -S < %s | FileCheck %s
+
+; CHECK: plus:
+; CHECK-NEXT:  %{{.*}} = call i64 @func2(i64* getelementptr inbounds (%struct, %struct* @Global, i32 0, i32 3))
+; CHECK: minus:
+; CHECK-NEXT: %{{.*}} = call i64 @func2(i64* getelementptr inbounds (%struct, %struct* @Global, i32 0, i32 4))
+
+%struct = type { i8, i16, i32, i64, i64}
+@Global = internal constant %struct {i8 0, i16 1, i32 2, i64 3, i64 4}
+define internal i64 @zoo(i1 %flag) {
+entry:
+  br i1 %flag, label %plus, label %minus
+
+plus:
+  %arg = getelementptr %struct, %struct* @Global, i32 0, i32 3
+  %tmp0 = call i64 @func2(i64* %arg)
+  br label %merge
+
+minus:
+  %arg2 = getelementptr %struct, %struct* @Global, i32 0, i32 4
+  %tmp1 = call i64 @func2(i64* %arg2)
+  br label %merge
+
+merge:
+  %tmp2 = phi i64 [ %tmp0, %plus ], [ %tmp1, %minus]
+  ret i64 %tmp2
+}
+
+define internal i64 @func2(i64 *%x) {
+entry:
+  %val = ptrtoint i64* %x to i64
+  ret i64 %val
+}
+
+define internal i64 @func(i64 *%x, i64 (i64*)* %binop) {
+entry:
+  %tmp0 = call i64 %binop(i64* %x)
+  ret i64 %tmp0
+}
+
+define i64 @main() {
+    %1 = call i64 @zoo(i1 0)
+    %2 = call i64 @zoo(i1 1)
+    %3 = add i64 %1, %2
+    ret i64 %3
+}
 No newline at end of file