[FuncSpec] Ignore ConstantExpr when find possible constant

[ChuanqiXu]

Summary

Now the function specialization pass would crash for SPEC2017int rate.

The reason would be we didn't update the SCCPSolver after we replace the constant.
It would be fixed once we adopted D106426.

Since it looks like that D106426 would take more time to check in, I think we could use this fix first.

Test Plan: check-all, SPEC2017 int rate

[ChuanqiXu]

Fix tests.

[ChuanqiXu]

BTW, may I ask a question here. The test case in this diff would get changed by function specialization pass but there wouldn't be any function get specialized. I know that the change is introduced by SCCPSolver. But do you think it is suitable? I feel it is a little bit strange.

[SjoerdMeijer]

In D107067#2915594, @ChuanqiXu wrote:

BTW, may I ask a question here. The test case in this diff would get changed by function specialization pass but there wouldn't be any function get specialized. I know that the change is introduced by SCCPSolver. But do you think it is suitable? I feel it is a little bit strange.

It's definitely an indirect way of testing things, which indeed is not ideal. It's also not ideal that IR gets changed while the transformation does not get triggered, but in this case that's not something we can avoid I think. The solver used to be integral part of the IPSCC transformation, and thus we are not testing this in isolation. Looks like the best we can do is add this test here.

Now that there's progress with D106426, we don't need the FIXME in the code (we definitely want the test)? This change depends on D106426, and we can commit them soon after each other?

[ChuanqiXu]

In D107067#2915783, @SjoerdMeijer wrote:

In D107067#2915594, @ChuanqiXu wrote:

BTW, may I ask a question here. The test case in this diff would get changed by function specialization pass but there wouldn't be any function get specialized. I know that the change is introduced by SCCPSolver. But do you think it is suitable? I feel it is a little bit strange.

It's definitely an indirect way of testing things, which indeed is not ideal. It's also not ideal that IR gets changed while the transformation does not get triggered, but in this case that's not something we can avoid I think. The solver used to be integral part of the IPSCC transformation, and thus we are not testing this in isolation. Looks like the best we can do is add this test here.

Yes. We could add it to the todo list.

Now that there's progress with D106426, we don't need the FIXME in the code (we definitely want the test)? This change depends on D106426, and we can commit them soon after each other?

Yeah.

[ChuanqiXu]

Discarded.

[SjoerdMeijer]

But is the test case still interesting to have?

[ChuanqiXu]

In D107067#2938967, @SjoerdMeijer wrote:

But is the test case still interesting to have?

My bad. The test case is checked in by D107622.

[SjoerdMeijer]

Ah okay, didn't realise that :-)

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

for (User *U : F->users()) {
       auto *V = CS.getArgOperand(A->getArgNo());
       // TrackValueOfGlobalVariable only tracks scalar global variables.
       if (auto *GV = dyn_cast<GlobalVariable>(V)) {
         if (!GV->getValueType()->isSingleValueType()) {
           return false;
         }
       }
 
+      /// FIXME: Remove this once we update the solver after we replace
+      /// the constant.
+      if (isa<ConstantExpr>(V))
+        continue;
+
       if (isa<Constant>(V) && (Solver.getLatticeValueFor(V).isConstant() ||
                                EnableSpecializationForLiteralConstant))
         Constants.push_back(cast<Constant>(V));
       else
         AllConstant = false;
     }
 
     // If the argument can only take on constant values, AllConstant will be

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

+; Test function specialization wouldn't crash due to constant expression.
+; RUN: opt -function-specialization -S < %s | FileCheck %s
+
+; CHECK-LABEL: @main(i64 %x, i1 %flag) {
+; CHECK:         entry:
+; CHECK-NEXT:      br i1 %flag, label %plus, label %minus
+; CHECK:         plus:
+; CHECK-NEXT:      [[TMP0:%.+]] = add i64 %x, 1
+; CHECH-NEXT:      br label %merge
+; CHECK:         minus:
+; CHECK-NEXT:      [[TMP1:%.+]] = sub i64 %x, 1
+; CHECK-NEXT:      br label %merge
+; CHECK:         merge:
+; CHECK-NEXT:      [[TMP2:%.+]] = phi i64 [ [[TMP0]], %plus ], [ [[TMP1]], %minus ]
+; CHECK-NEXT:      ret i64 [[TMP2]]
+; CHECK-NEXT:  }
+;
+
+%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