[FuncSpec] Partially revert rG8b360c69e9e3.

[labrinea]

Summary

The patch "Fix assertion failure when value is not added to solver" removed code which made the pass specialize in cases we were not intending before. The test function-specialization-constant-expression.ll explicitely mentions that in a comment. When I rebase my patch https://reviews.llvm.org/D126455 on top of main I get the same assertion error when compiling this test file. Adding back the removed code fixes the problem.

[ChuanqiXu]

LGTM basically. @sinan would you like to take a look?

[sinan]

The reason why test cases crash at Solver.getLatticeValueFor is that SCCPSolver does not track the lattice value of a callbase argument. However, it is doable for SCCPSolver to track the lattice value of a ConstantExpr, and that is why I think skipping ConstantExpr is not an sound solution.

So, the potential problem with D126455 is that the argument of a callbase is changed before FS, and SCCPSolver is not updated. Can you have a check on that?

[labrinea]

In D128822#3644296, @sinan wrote:

The reason why test cases crash at Solver.getLatticeValueFor is that SCCPSolver does not track the lattice value of a callbase argument. However, it is doable for SCCPSolver to track the lattice value of a ConstantExpr, and that is why I think skipping ConstantExpr is not an sound solution.

So, the potential problem with D126455 is that the argument of a callbase is changed before FS, and SCCPSolver is not updated. Can you have a check on that?

Hi, thanks for the input. D126455 is part of a patch series. Indeed if the users of a replaced value are visited then the crash doesn't happen. I am already doing this on D128825 and D126456.

[labrinea]

Ping. I wanted to implement the idea discussed on https://discourse.llvm.org/t/rfc-should-we-enable-function-specialization/61518 in incremental steps, not in one go. The problem @sinan mentions is fixed in later patches. Does it make sense to land this patch first and once the fix is in place to revert the change in getPossibleConstants() and adapt the test to reflect that we actually intend to specialize?

[sinan]

Sounds fine. I tend to accept this patch since it is a stepping stone for the patch series, and we can revisit the constant expression case later after all patches merged.

[labrinea]

Cheers

[labrinea]

Abandoning in favor of D126455.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

for (User *U : F->users()) {
       // to worry about the passed value.
       if (!Solver.isBlockExecutable(CS.getParent()))
         continue;
 
       auto *V = CS.getArgOperand(A->getArgNo());
       if (isa<PoisonValue>(V))
         return;
 
+      // For now, constant expressions are fine but only if they are function
+      // calls.
+      if (auto *CE = dyn_cast<ConstantExpr>(V))
+        if (!isa<Function>(CE->getOperand(0)))
+          return;
+
       // TrackValueOfGlobalVariable only tracks scalar global variables.
       if (auto *GV = dyn_cast<GlobalVariable>(V)) {
         // Check if we want to specialize on the address of non-constant
         // global values.
         if (!GV->isConstant())
           if (!SpecializeOnAddresses)
             return;
 

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

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 
 ; 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 -force-function-specialization -S < %s | FileCheck %s
 
 %struct = type { i8, i16, i32, i64, i64}
 @Global = internal constant %struct {i8 0, i16 1, i32 2, i64 3, i64 4}
 
 define internal i64 @func2(i64 *%x) {
+; CHECK-LABEL: @func2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[VAL:%.*]] = ptrtoint i64* [[X:%.*]] to i64
+; CHECK-NEXT:    ret i64 [[VAL]]
+;
 entry:
   %val = ptrtoint i64* %x to i64
   ret i64 %val
 }
 
 define internal i64 @func(i64 *%x, i64 (i64*)* %binop) {
 ; CHECK-LABEL: @func(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = call i64 [[BINOP:%.*]](i64* [[X:%.*]])
 ; CHECK-NEXT:    ret i64 [[TMP0]]
 ;
 entry:
   %tmp0 = call i64 %binop(i64* %x)
   ret i64 %tmp0
 }
 
 define internal i64 @zoo(i1 %flag) {
 ; CHECK-LABEL: @zoo(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br i1 [[FLAG:%.*]], label [[PLUS:%.*]], label [[MINUS:%.*]]
 ; CHECK:       plus:
-; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @func2.2(i64* getelementptr inbounds ([[STRUCT:%.*]], %struct* @Global, i32 0, i32 3))
+; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @func2(i64* getelementptr inbounds ([[STRUCT:%.*]], %struct* @Global, i32 0, i32 3))
 ; CHECK-NEXT:    br label [[MERGE:%.*]]
 ; CHECK:       minus:
-; CHECK-NEXT:    [[TMP1:%.*]] = call i64 @func2.1(i64* getelementptr inbounds ([[STRUCT]], %struct* @Global, i32 0, i32 4))
+; CHECK-NEXT:    [[TMP1:%.*]] = call i64 @func2(i64* getelementptr inbounds ([[STRUCT]], %struct* @Global, i32 0, i32 4))
 ; CHECK-NEXT:    br label [[MERGE]]
 ; CHECK:       merge:
 ; CHECK-NEXT:    [[TMP2:%.*]] = phi i64 [ [[TMP0]], [[PLUS]] ], [ [[TMP1]], [[MINUS]] ]
 ; CHECK-NEXT:    ret i64 [[TMP2]]
 ;
 entry:
   br i1 %flag, label %plus, label %minus