[PatternMatch] Do not match constant expressions for binops

[nikic]

Summary

Currently, m_Mul style matchers also match constant expressions. This is a regular source of assertion failures (usually by trying to do a match and then cast to Instruction or BinaryOperator) and infinite combine loops. At the same time, I don't think this provides useful optimization capabilities (all of the tests affected here are regression tests for crashes / infinite loops).

Long term, all of these constant expressions (apart from add/sub possibly) are slated for removal per https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179 -- but doing those removals can itself expose new crashed and infinite loops due to the current PatternMatch behavior. I've been working on removing and expressions, and I've already run into 3 issues of that kind.

[goldstein.w.n]

Is there a way to check/get a ConstantExpr as a ImmConstant? If so maybe this should come with an update to m_ImmConstant to also try to extract from a ConstantExpr match.

[nikic]

In D156401#4539763, @goldstein.w.n wrote:

Is there a way to check/get a ConstantExpr as a ImmConstant? If so maybe this should come with an update to m_ImmConstant to also try to extract from a ConstantExpr match.

I don't really get what you mean here. Isn't m_ImmConstant specifically about not matching constant expressions? m_Constant() is the way to match arbitrary constants (including ConstantExpr).

[goldstein.w.n]

In D156401#4539923, @nikic wrote:

In D156401#4539763, @goldstein.w.n wrote:

Is there a way to check/get a ConstantExpr as a ImmConstant? If so maybe this should come with an update to m_ImmConstant to also try to extract from a ConstantExpr match.

I don't really get what you mean here. Isn't m_ImmConstant specifically about not matching constant expressions? m_Constant() is the way to match arbitrary constants (including ConstantExpr).

Err yeah nvm I misread some of the test changes.

Sorry.

Code change LGTM.

Regarding the regressions, as long as its temporary state its fine imo.
Might want to give others a day or so to comment as to that affect, however.

llvm/include/llvm/IR/PatternMatch.h

struct BinaryOp_match {
 
   template <typename OpTy> inline bool match(unsigned Opc, OpTy *V) {
     if (V->getValueID() == Value::InstructionVal + Opc) {
       auto *I = cast<BinaryOperator>(V);
       return (L.match(I->getOperand(0)) && R.match(I->getOperand(1))) ||
              (Commutable && L.match(I->getOperand(1)) &&
               R.match(I->getOperand(0)));
     }
-    if (auto *CE = dyn_cast<ConstantExpr>(V))
-      return CE->getOpcode() == Opc &&
-             ((L.match(CE->getOperand(0)) && R.match(CE->getOperand(1))) ||
-              (Commutable && L.match(CE->getOperand(1)) &&
-               R.match(CE->getOperand(0))));
     return false;
   }
 
   template <typename OpTy> bool match(OpTy *V) { return match(Opcode, V); }
 };
 
 template <typename LHS, typename RHS>
 inline BinaryOp_match<LHS, RHS, Instruction::Add> m_Add(const LHS &L,
struct BinOpPred_match : Predicate {
   RHS_t R;
 
   BinOpPred_match(const LHS_t &LHS, const RHS_t &RHS) : L(LHS), R(RHS) {}
 
   template <typename OpTy> bool match(OpTy *V) {
     if (auto *I = dyn_cast<Instruction>(V))
       return this->isOpType(I->getOpcode()) && L.match(I->getOperand(0)) &&
              R.match(I->getOperand(1));
-    if (auto *CE = dyn_cast<ConstantExpr>(V))
-      return this->isOpType(CE->getOpcode()) && L.match(CE->getOperand(0)) &&
-             R.match(CE->getOperand(1));
     return false;
   }
 };
 
 struct is_shift_op {
   bool isOpType(unsigned Opcode) { return Instruction::isShift(Opcode); }
 };
 

llvm/test/Transforms/InstCombine/mul-inseltpoison.ll

;
   %D = zext i32 %B to i64
   %E = mul i64 %C, %D
   ret i64 %E
 }
 
 @PR22087 = external global i32
 define i32 @test31(i32 %V) {
 ; CHECK-LABEL: @test31(
-; CHECK-NEXT:    [[MUL1:%.*]] = shl i32 [[V:%.*]], zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32)
-; CHECK-NEXT:    ret i32 [[MUL1]]
+; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[V:%.*]], shl (i32 1, i32 zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32))
+; CHECK-NEXT:    ret i32 [[MUL]]
 ;
   %mul = mul i32 %V, shl (i32 1, i32 zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32))
   ret i32 %mul
 }
 
 define i32 @test32(i32 %X) {
 ; CHECK-LABEL: @test32(
 ; CHECK-NEXT:    [[MUL:%.*]] = shl i32 [[X:%.*]], 31

 }
 
 @X = global i32 5
 
 define i64 @test_mul_canonicalize_neg_is_not_undone(i64 %L1) {
 ; Check we do not undo the canonicalization of 0 - (X * Y), if Y is a constant
 ; expr.
 ; CHECK-LABEL: @test_mul_canonicalize_neg_is_not_undone(
-; CHECK-NEXT:    [[TMP1:%.*]] = mul i64 [[L1:%.*]], ptrtoint (ptr @X to i64)
-; CHECK-NEXT:    [[B4:%.*]] = sub i64 0, [[TMP1]]
+; CHECK-NEXT:    [[B4:%.*]] = mul i64 [[L1:%.*]], sub (i64 0, i64 ptrtoint (ptr @X to i64))
 ; CHECK-NEXT:    ret i64 [[B4]]
 ;
   %v1 = ptrtoint ptr @X to i64
   %B8 = sub i64 0, %v1
   %B4 = mul i64 %B8, %L1
   ret i64 %B4
 }
 

llvm/test/Transforms/InstCombine/mul.ll

;
   %D = zext i32 %B to i64
   %E = mul i64 %C, %D
   ret i64 %E
 }
 
 @PR22087 = external global i32
 define i32 @test31(i32 %V) {
 ; CHECK-LABEL: @test31(
-; CHECK-NEXT:    [[MUL1:%.*]] = shl i32 [[V:%.*]], zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32)
-; CHECK-NEXT:    ret i32 [[MUL1]]
+; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[V:%.*]], shl (i32 1, i32 zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32))
+; CHECK-NEXT:    ret i32 [[MUL]]
 ;
   %mul = mul i32 %V, shl (i32 1, i32 zext (i1 icmp ne (ptr inttoptr (i64 1 to ptr), ptr @PR22087) to i32))
   ret i32 %mul
 }
 
 define i32 @test32(i32 %X) {
 ; CHECK-LABEL: @test32(
 ; CHECK-NEXT:    [[MUL:%.*]] = shl i32 [[X:%.*]], 31

 }
 
 @X = global i32 5
 
 define i64 @test_mul_canonicalize_neg_is_not_undone(i64 %L1) {
 ; Check we do not undo the canonicalization of 0 - (X * Y), if Y is a constant
 ; expr.
 ; CHECK-LABEL: @test_mul_canonicalize_neg_is_not_undone(
-; CHECK-NEXT:    [[TMP1:%.*]] = mul i64 [[L1:%.*]], ptrtoint (ptr @X to i64)
-; CHECK-NEXT:    [[B4:%.*]] = sub i64 0, [[TMP1]]
+; CHECK-NEXT:    [[B4:%.*]] = mul i64 [[L1:%.*]], sub (i64 0, i64 ptrtoint (ptr @X to i64))
 ; CHECK-NEXT:    ret i64 [[B4]]
 ;
   %v1 = ptrtoint ptr @X to i64
   %B8 = sub i64 0, %v1
   %B4 = mul i64 %B8, %L1
   ret i64 %B4
 }
 

llvm/test/Transforms/InstCombine/pr32686.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -passes=instcombine %s | FileCheck %s
 
 @a = common global i8 0, align 1
 @b = external global i32
 
 define void @tinkywinky() {
 ; CHECK-LABEL: @tinkywinky(
 ; CHECK-NEXT:    [[PATATINO:%.*]] = load i8, ptr @a, align 1
 ; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i8 [[PATATINO]], 0
-; CHECK-NEXT:    [[TMP1:%.*]] = zext i1 [[TOBOOL_NOT]] to i32
-; CHECK-NEXT:    [[OR1:%.*]] = or i32 [[TMP1]], or (i32 zext (i1 icmp ne (ptr @a, ptr @b) to i32), i32 2)
-; CHECK-NEXT:    store i32 [[OR1]], ptr @b, align 4
+; CHECK-NEXT:    [[LNOT_EXT:%.*]] = zext i1 [[TOBOOL_NOT]] to i32
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[LNOT_EXT]], xor (i32 zext (i1 icmp ne (ptr @a, ptr @b) to i32), i32 2)
+; CHECK-NEXT:    store i32 [[OR]], ptr @b, align 4
 ; CHECK-NEXT:    ret void
 ;
   %patatino = load i8, ptr @a
   %tobool = icmp ne i8 %patatino, 0
   %lnot = xor i1 %tobool, true
   %lnot.ext = zext i1 %lnot to i32
   %or = or i32 xor (i32 zext (i1 icmp ne (ptr @a, ptr @b) to i32), i32 2), %lnot.ext
   store i32 %or, ptr @b, align 4
   ret void
 }

llvm/test/Transforms/InstCombine/switch-constant-expr.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
 
 @g = global i32 0
 
 ; PR30486
 define i32 @single_case() {
 ; CHECK-LABEL: @single_case(
-; CHECK-NEXT:    switch i32 ptrtoint (ptr @g to i32), label [[X:%.*]] [
+; CHECK-NEXT:    switch i32 add (i32 ptrtoint (ptr @g to i32), i32 -1), label [[X:%.*]] [
 ; CHECK-NEXT:    ]
 ; CHECK:       x:
 ; CHECK-NEXT:    ret i32 0
 ;
   switch i32 add (i32 ptrtoint (ptr @g to i32), i32 -1), label %x []
 x:
   ret i32 0
 }
 
 define i32 @multiple_cases() {
 ; CHECK-LABEL: @multiple_cases(
-; CHECK-NEXT:    switch i32 ptrtoint (ptr @g to i32), label [[X:%.*]] [
-; CHECK-NEXT:    i32 2, label [[ONE:%.*]]
-; CHECK-NEXT:    i32 3, label [[TWO:%.*]]
+; CHECK-NEXT:    switch i32 add (i32 ptrtoint (ptr @g to i32), i32 -1), label [[X:%.*]] [
+; CHECK-NEXT:    i32 1, label [[ONE:%.*]]
+; CHECK-NEXT:    i32 2, label [[TWO:%.*]]
 ; CHECK-NEXT:    ]
 ; CHECK:       x:
 ; CHECK-NEXT:    ret i32 0
 ; CHECK:       one:
 ; CHECK-NEXT:    ret i32 1
 ; CHECK:       two:
 ; CHECK-NEXT:    ret i32 2
 ;

llvm/test/Transforms/InstCombine/udiv-simplify.ll

 
 ; The udiv should be simplified according to the rule:
 ; X udiv (C1 << N), where C1 is `1<<C2` --> X >> (N+C2)
 @b = external global [1 x i16]
 
 define i32 @PR30366(i1 %a) {
 ; CHECK-LABEL: @PR30366(
 ; CHECK-NEXT:    [[Z:%.*]] = zext i1 [[A:%.*]] to i32
-; CHECK-NEXT:    [[D1:%.*]] = lshr i32 [[Z]], zext (i16 ptrtoint (ptr @b to i16) to i32)
-; CHECK-NEXT:    ret i32 [[D1]]
+; CHECK-NEXT:    [[D:%.*]] = udiv i32 [[Z]], zext (i16 shl (i16 1, i16 ptrtoint (ptr @b to i16)) to i32)
+; CHECK-NEXT:    ret i32 [[D]]
 ;
   %z = zext i1 %a to i32
   %d = udiv i32 %z, zext (i16 shl (i16 1, i16 ptrtoint (ptr @b to i16)) to i32)
   ret i32 %d
 }
 
 ; OSS-Fuzz #4857
 ; https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4857