[FuncSpec] Don't crash and specialise on poison or constexpr values

[SjoerdMeijer]

Summary

Function specialization was crashing at i) poison values and 2) constexpr values.

The problem is that these values are not added to the solver, so when a lookup is performed the crash happens. This fixes that by not specialising on these values. For poison that is obvious, but for constexpr this is a change in behaviour. Thus, in one way this is a bit of a stopgap, but specialising on constexpr values wasn't done very intentionally, and need some more work and tests if we wanted to support this, which is why not allowing constexprs seems fine to me.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=51600

[ChuanqiXu]

I met the problem about ConstantExpr before: https://reviews.llvm.org/D107067. So it looks good to me basically. But I am wondering if it is better to fix the solver to solve these kind of problem fundamentally?

[SjoerdMeijer]

In D110529#3023974, @ChuanqiXu wrote:

I met the problem about ConstantExpr before: https://reviews.llvm.org/D107067. So it looks good to me basically. But I am wondering if it is better to fix the solver to solve these kind of problem fundamentally?

Yep, agreed. That's why I called this change to function specialisation a stopgap, but I think this makes sense because I don't think we want to support constexprs at this point.

In addition, we would indeed need to fix the solver (or how it is used), which I think is a separate issue. I don't know if the solver should exit more gracefully and returns a "don't know", and/or really support these constexprs. That needs some looking into...

[ChuanqiXu]

In D110529#3023997, @SjoerdMeijer wrote:

In D110529#3023974, @ChuanqiXu wrote:

I met the problem about ConstantExpr before: https://reviews.llvm.org/D107067. So it looks good to me basically. But I am wondering if it is better to fix the solver to solve these kind of problem fundamentally?

Yep, agreed. That's why I called this change to function specialisation a stopgap, but I think this makes sense because I don't think we want to support constexprs at this point.

In addition, we would indeed need to fix the solver (or how it is used), which I think is a separate issue. I don't know if the solver should exit more gracefully and returns a "don't know", and/or really support these constexprs. That needs some looking into...

Agreed. I think we should add corresponding TODO or FIXME in SCCPSolver.cpp in this patch or in another NFC patch.

[SjoerdMeijer]

In D110529#3024013, @ChuanqiXu wrote:

Agreed. I think we should add corresponding TODO or FIXME in SCCPSolver.cpp in this patch or in another NFC patch.

Ok, thanks, I will commit this first. Then I will have a look later this week at the solver and when I understand better where things happen, leave a comment somewhere (or propose a fix).

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

for (User *U : F->users()) {
       }
 
       // 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 false;
+      if (isa<ConstantExpr>(V))
+        return false;
+
       // 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 false;
 

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.1(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.2(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
 
 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) {
-; 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 i64 @main() {
 ; CHECK-LABEL: @main(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call i64 @zoo(i1 false)
 ; CHECK-NEXT:    [[TMP2:%.*]] = call i64 @zoo(i1 true)
 ; CHECK-NEXT:    [[TMP3:%.*]] = add i64 [[TMP1]], [[TMP2]]
 ; CHECK-NEXT:    ret i64 [[TMP3]]
 ;
   %1 = call i64 @zoo(i1 0)
   %2 = call i64 @zoo(i1 1)
   %3 = add i64 %1, %2
   ret i64 %3
 }
-
-; CHECK-LABEL: define internal i64 @func2.1(
-; CHECK-NEXT:  entry:
-; CHECK-NEXT:    ret i64 ptrtoint (i64* getelementptr inbounds (%struct, %struct* @Global, i32 0, i32 3) to i64)
-; CHECK-NEXT:  }
-
-; CHECK-LABEL: define internal i64 @func2.2(
-; CHECK-NEXT:  entry:
-; CHECK-NEXT:    ret i64 ptrtoint (i64* getelementptr inbounds (%struct, %struct* @Global, i32 0, i32 4) to i64)
-; CHECK-NEXT:  }

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

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -function-specialization -force-function-specialization -S < %s | FileCheck %s
+
+; Check that we don't crash and specialise on a constant expression.
+
+%struct.pluto = type { %struct.spam }
+%struct.quux = type { i16 }
+%struct.spam = type { i16 }
+
+@global.5 = external dso_local global [4 x %struct.pluto], align 1
+@global.12 = external global %struct.quux, align 1
+
+define internal i16 @wobble.972(%struct.quux* byval(%struct.quux) align 1 %arg, i16 %arg1, %struct.spam* byval(%struct.spam) align 1 %arg2, %struct.quux* byval(%struct.quux) align 1 %arg3) #4 {
+; CHECK-LABEL: @wobble.972(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP34:%.*]] = bitcast %struct.spam* [[ARG2:%.*]] to i16*
+; CHECK-NEXT:    unreachable
+;
+bb:
+  %tmp34 = bitcast %struct.spam* %arg2 to i16*
+  unreachable
+}
+
+define internal i16 @snork() {
+; CHECK-LABEL: @snork(
+; CHECK-NEXT:  bb4:
+; CHECK-NEXT:    [[TMP:%.*]] = call i16 @wobble.972(%struct.quux* byval([[STRUCT_QUUX:%.*]]) align 1 undef, i16 undef, %struct.spam* byval([[STRUCT_SPAM:%.*]]) align 1 getelementptr inbounds ([4 x %struct.pluto], [4 x %struct.pluto]* @global.5, i32 0, i32 3, i32 0), %struct.quux* byval([[STRUCT_QUUX]]) align 1 @global.12)
+; CHECK-NEXT:    unreachable
+;
+bb4:
+  %tmp = call i16 @wobble.972(%struct.quux* byval(%struct.quux) align 1 undef, i16 undef, %struct.spam* byval(%struct.spam) align 1 getelementptr inbounds ([4 x %struct.pluto], [4 x %struct.pluto]* @global.5, i32 0, i32 3, i32 0), %struct.quux* byval(%struct.quux) align 1 @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
+}

llvm/test/Transforms/FunctionSpecialization/function-specialization-poison.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -function-specialization -force-function-specialization -S < %s | FileCheck %s
+
+; Check that we don't crash and specialise on a poison value.
+
+%struct.quux = type { i16 }
+%struct.spam = type { i16 }
+
+@global.12 = external global %struct.quux, align 1
+
+define internal i16 @wobble(%struct.spam* byval(%struct.spam) %arg2) {
+; CHECK-LABEL: @wobble(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[C:%.*]] = bitcast %struct.spam* [[ARG2:%.*]] to i16*
+; CHECK-NEXT:    unreachable
+;
+bb:
+  %C = bitcast %struct.spam* %arg2 to i16*
+  unreachable
+}
+
+define internal i16 @snork() {
+; CHECK-LABEL: @snork(
+; CHECK-NEXT:  bb4:
+; CHECK-NEXT:    [[B:%.*]] = call i16 @wobble(%struct.spam* poison)
+; CHECK-NEXT:    unreachable
+;
+bb4:
+  %B = call i16 @wobble(%struct.spam* poison)
+  unreachable
+}
+
+define i16 @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[A:%.*]] = call i16 @snork()
+; CHECK-NEXT:    unreachable
+;
+bb:
+  %A = call i16 @snork()
+  unreachable
+}