[FuncSpec]Fix assertion failure when lattice value is not found.

[sinan]

Summary

The patch from D110529 partially solves the problem of bug51600 . However, the reason for crashing is that if an argument is byval, and if the function is not readonly, 'ValueState' will not record the argument(in SCCPInstVisitor::handleCallArguments), and thus we cannot find its value state (in getPossibleConstants) and this leads to an assertation failure or crash.

The problem still exists in the current codebase, and this patch can solve it and also relax the constraint on constant expression. The attached test cases *-expression4.ll and *-expression5.ll can bypass the check of global variable and constant expression and trigger assertation failure.

[ChuanqiXu]

We should generate a patch with option -U9999 to offer more context to review.

Thanks for working on this. Have you measured any performance/code size change?

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

[sinan]

In D126355#3536597, @ChuanqiXu wrote:

We should generate a patch with option -U9999 to offer more context to review.

Thanks for the info and feedback. I have updated the diff file.

Thanks for working on this. Have you measured any performance/code size change?

I only ran on Spec 2017, and luckily no crash during compiling. I will check the performance/code size different later.

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Ok, I will think about it, thanks again!

[bin.cheng]

In D126355#3539163, @sinan wrote:

In D126355#3536597, @ChuanqiXu wrote:

We should generate a patch with option -U9999 to offer more context to review.

Thanks for the info and feedback. I have updated the diff file.

Thanks for working on this. Have you measured any performance/code size change?

I only ran on Spec 2017, and luckily no crash during compiling. I will check the performance/code size different later.

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Ok, I will think about it, thanks again!

Given it might be non-trivial to support ByVal arguments in SCCPSolver, and ByVal is generally used for pointers to structure/array, we can first try to get this version of patch reviewed and merged, then work on ByVal/SCCPSolver as a following task (probably with a lower priority).

[sinan]

In D126355#3536597, @ChuanqiXu wrote:

We should generate a patch with option -U9999 to offer more context to review.

Thanks for working on this. Have you measured any performance/code size change?

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Hi,

I tested with flag -Wl,-mllvm,--enable-function-specialization on Spec 2017. No noticeable regression in compilation time(measured by -fproc-stat-report) and performance was found.

[ChuanqiXu]

LGTM then.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

private:
   }
 
   /// Collect in \p Constants all the constant values that argument \p A can
   /// take on.
   void getPossibleConstants(Argument *A,
                             SmallVectorImpl<CallArgBinding> &Constants) {
     Function *F = A->getParent();
 
+    // SCCP solver does not record an argument that will be constructed on
+    // stack.
+    if (A->hasByValAttr() && !F->onlyReadsMemory())
+      return;
+
     // Iterate over all the call sites of the argument's parent function.
     for (User *U : F->users()) {
       if (!isa<CallInst>(U) && !isa<InvokeInst>(U))
         continue;
       auto &CS = *cast<CallBase>(U);
       // If the call site has attribute minsize set, that callsite won't be
       // specialized.
       if (CS.hasFnAttr(Attribute::MinSize))
         continue;
 
       // If the parent of the call site will never be executed, we don't need
       // 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(i64* getelementptr inbounds ([[STRUCT:%.*]], %struct* @Global, i32 0, i32 3))
+; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @func2.2(i64* getelementptr inbounds ([[STRUCT:%.*]], %struct* @Global, i32 0, i32 3))
 ; CHECK-NEXT:    br label [[MERGE:%.*]]
 ; CHECK:       minus:
-; CHECK-NEXT:    [[TMP1:%.*]] = call i64 @func2(i64* getelementptr inbounds ([[STRUCT]], %struct* @Global, i32 0, i32 4))
+; CHECK-NEXT:    [[TMP1:%.*]] = call i64 @func2.1(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
 

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

+; RUN: opt -function-specialization -force-function-specialization -S < %s | FileCheck %s
+
+; Check that we don't crash and specialise on a function call with byval attribute.
+
+; CHECK-NOT: wombat.{{[0-9]+}}
+
+declare i32* @quux()
+declare i32* @eggs()
+
+define i32 @main() {
+; CHECK:       bb:
+; CHECK-NEXT:    tail call void @wombat(i8* undef, i64 undef, i64 undef, i32* byval(i32) bitcast (i32* ()* @quux to i32*))
+; CHECK-NEXT:    tail call void @wombat(i8* undef, i64 undef, i64 undef, i32* byval(i32) bitcast (i32* ()* @eggs to i32*))
+; CHECK-NEXT:    ret i32 undef
+;
+bb:
+  tail call void @wombat(i8* undef, i64 undef, i64 undef, i32* byval(i32) bitcast (i32* ()* @quux to i32*))
+  tail call void @wombat(i8* undef, i64 undef, i64 undef, i32* byval(i32) bitcast (i32* ()* @eggs to i32*))
+  ret i32 undef
+}
+
+define internal void @wombat(i8* %arg, i64 %arg1, i64 %arg2, i32* byval(i32) %func) {
+; CHECK:       bb2:
+; CHECK-NEXT:    [[FUNPTR:%.*]] = bitcast i32* %func to i32* (i8*, i8*)*
+; CHECK-NEXT:    [[TMP:%.*]] = tail call i32* [[FUNPTR]](i8* undef, i8* undef)
+; CHECK-NEXT:    ret void
+;
+bb2:
+  %mycall = bitcast i32* %func to i32* (i8*, i8*)*
+  %tmp = tail call i32* %mycall(i8* undef, i8* undef)
+  ret void
+}

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

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -function-specialization -force-function-specialization -func-specialization-on-address -S < %s | FileCheck %s
+
+; Check that we don't crash and specialise on a scalar global variable with byval attribute.
+
+; CHECK-NOT: wombat.{{[0-9]+}}
+
+%struct.pluto = type { %struct.spam }
+%struct.quux = type { i16 }
+%struct.spam = type { i16 }
+
+@global.5 = external dso_local global i128
+@global.12 = external global %struct.quux
+
+define internal i16 @wobble(%struct.quux* %arg, i16 %arg1, i128* byval(i128) %arg2, %struct.quux* %arg3) {
+; CHECK-LABEL: @wobble(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP34:%.*]] = bitcast i128* [[ARG2:%.*]] to i16*
+; CHECK-NEXT:    unreachable
+;
+bb:
+  %tmp34 = bitcast i128* %arg2 to i16*
+  unreachable
+}
+
+define internal i16 @snork() {
+; CHECK-LABEL: @snork(
+; CHECK-NEXT:  bb4:
+; CHECK-NEXT:    [[TMP35:%.*]] = call i16 @wobble(%struct.quux* undef, i16 2, i128* byval(i128) @global.5, %struct.quux* @global.12)
+; CHECK-NEXT:    unreachable
+;
+bb4:
+  %tmp35 = call i16 @wobble(%struct.quux* undef, i16 2, i128* byval(i128) @global.5, %struct.quux* @global.12)
+  unreachable
+}
+
+define i16 @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP:%.*]] = call i16 @snork()
+; CHECK-NEXT:    unreachable
+;
+bb:
+  %tmp = call i16 @snork()
+  unreachable
+}