[RISCV] Optimize (add (mul x, c0), c1)

[benshi001]

Summary

Optimize (add (mul x, c0), c1) -> (ADDI (MUL (ADDI, c1/c0), c0), c1%c0),
if c1/c0 and c1%c0 are simm12, while c1 is not.

Optimize (add (mul x, c0), c1) -> (MUL (ADDI, c1/c0), c0),
if c1%c0 is zero, and c1/c0 is simm12 while c1 is not.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5864

You just use isInt<12> from MathUtils.h and not use APInt.

5864

You need to guard against C0 being 0.

I'm also concerned about the possibility of signed overflow if C1=INT_MIN C0 = -1.

5875

No need for else after a return.

[benshi001]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5864

I think we can skip if C1 is -1/0/+1, then all corner cases are excluded.

[benshi001]

Thanks, I have changed according to your comments.

What's more, I think for c1==1/0/-1, I can just skip and let other passes handle it.

[benshi001]

ping ...

[benshi001]

ping ...

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

6193

Please put this call in performADDCombine. The idea was that all ADD specific things should be in one place. This switch is getting pretty crazy.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5890

VT is guaranteed integer since you're starting from an integer add instruction.

5898

I believe N0C and N1C are more consistent with other DAGCombiner code.

5926

I don't like the name N0 here. SelectionDAG code very often uses N0 to mean N->getOperand(0).

I'd suggest

if (SDValue V = transformAddImmMulImm(N, DAG, Subtarget))
  return V;

V is more generic name and putting in the declare in the if limits its scope.

[craig.topper]

LGTM

[nathanchance]

This commit causes an infinite loop when compiling the Linux kernel:

$ git bisect log
# bad: [43d6991c2a4cc2ac374e68c029634f2b59ffdfdf] [IR] Look through bitcast in hasFnAttribute()
# good: [6a2c2263fbca07a59b9f41181c1418df052e24d1] [X86] Improve i8 all-ones element insertion in pre-SSE4.1
git bisect start '43d6991c2a4cc2ac374e68c029634f2b59ffdfdf' '6a2c2263fbca07a59b9f41181c1418df052e24d1'
# good: [792101fff749191dfd4dadabe2ecd30a4d8cd973] [RISCV] Add test cases for missed opportunity to use vfmacc.vf. NFC
git bisect good 792101fff749191dfd4dadabe2ecd30a4d8cd973
# good: [20b58855e0cfc263d609e8bb59e692024ecb42aa] [CodeGen] SelectionDAGBuilder - Use const-ref iterator in for-range loops. NFCI.
git bisect good 20b58855e0cfc263d609e8bb59e692024ecb42aa
# bad: [08ef71ca92d9bd474ac4bbb5109e9ced19c77f0e] [InstCombine] move/add tests for trunc-of-lshr; NFC
git bisect bad 08ef71ca92d9bd474ac4bbb5109e9ced19c77f0e
# bad: [744ec74b305a5039dc74e2d043e1c136e06beac1] [NFC] `goto fail` has failed us in the past...
git bisect bad 744ec74b305a5039dc74e2d043e1c136e06beac1
# good: [b8e7f5320825812fc4b597ee5df3dccc53fe78bb] [AMDGPU][MC][GFX10] Enabled dlc for FLAT and GLOBAL atomics
git bisect good b8e7f5320825812fc4b597ee5df3dccc53fe78bb
# good: [ee31ad0ab5f7d443b0bd582582a3524dcf7f13f0] [clang-offload-bundler][docs][NFC] Add archive unbundling documentation
git bisect good ee31ad0ab5f7d443b0bd582582a3524dcf7f13f0
# good: [32b994bca66641cdac8586f25315daf349921ebc] [OpenCL] Defines helper function for OpenCL default address space
git bisect good 32b994bca66641cdac8586f25315daf349921ebc
# bad: [b3052013b43617defd777b1f3b4339540c4c07df] [RISCV] Optimize (add (mul x, c0), c1)
git bisect bad b3052013b43617defd777b1f3b4339540c4c07df
# first bad commit: [b3052013b43617defd777b1f3b4339540c4c07df] [RISCV] Optimize (add (mul x, c0), c1)

$ cat evergreen.i
int evergreen_dram_bandwidth_for_display_wm_0,
    evergreen_dram_bandwidth_for_display_disp_dram_allocation_0,
    evergreen_dram_bandwidth_for_display_bandwidth_0,
    evergreen_dram_bandwidth_for_display_a_0;
void evergreen_dram_bandwidth_for_display() {
  evergreen_dram_bandwidth_for_display_a_0 = 1000 << 12;
  evergreen_dram_bandwidth_for_display_disp_dram_allocation_0 = 3 << 12;
  long tmp = evergreen_dram_bandwidth_for_display_disp_dram_allocation_0 << 13;
  tmp = tmp / evergreen_dram_bandwidth_for_display_a_0;
  evergreen_dram_bandwidth_for_display_bandwidth_0 =
      evergreen_dram_bandwidth_for_display_wm_0 * tmp + 2048;
}

$ timeout 10s clang -O2 --target=riscv64-linux-gnu -c -o /dev/null evergreen.i

$ echo $?
124

This is still reproducible at current ToT (3778c1cd6ef5a3286d5d49e842a2c65fffb8f3a6).

[craig.topper]

In D108607#3018141, @nathanchance wrote:

This commit causes an infinite loop when compiling the Linux kernel:

$ git bisect log
# bad: [43d6991c2a4cc2ac374e68c029634f2b59ffdfdf] [IR] Look through bitcast in hasFnAttribute()
# good: [6a2c2263fbca07a59b9f41181c1418df052e24d1] [X86] Improve i8 all-ones element insertion in pre-SSE4.1
git bisect start '43d6991c2a4cc2ac374e68c029634f2b59ffdfdf' '6a2c2263fbca07a59b9f41181c1418df052e24d1'
# good: [792101fff749191dfd4dadabe2ecd30a4d8cd973] [RISCV] Add test cases for missed opportunity to use vfmacc.vf. NFC
git bisect good 792101fff749191dfd4dadabe2ecd30a4d8cd973
# good: [20b58855e0cfc263d609e8bb59e692024ecb42aa] [CodeGen] SelectionDAGBuilder - Use const-ref iterator in for-range loops. NFCI.
git bisect good 20b58855e0cfc263d609e8bb59e692024ecb42aa
# bad: [08ef71ca92d9bd474ac4bbb5109e9ced19c77f0e] [InstCombine] move/add tests for trunc-of-lshr; NFC
git bisect bad 08ef71ca92d9bd474ac4bbb5109e9ced19c77f0e
# bad: [744ec74b305a5039dc74e2d043e1c136e06beac1] [NFC] `goto fail` has failed us in the past...
git bisect bad 744ec74b305a5039dc74e2d043e1c136e06beac1
# good: [b8e7f5320825812fc4b597ee5df3dccc53fe78bb] [AMDGPU][MC][GFX10] Enabled dlc for FLAT and GLOBAL atomics
git bisect good b8e7f5320825812fc4b597ee5df3dccc53fe78bb
# good: [ee31ad0ab5f7d443b0bd582582a3524dcf7f13f0] [clang-offload-bundler][docs][NFC] Add archive unbundling documentation
git bisect good ee31ad0ab5f7d443b0bd582582a3524dcf7f13f0
# good: [32b994bca66641cdac8586f25315daf349921ebc] [OpenCL] Defines helper function for OpenCL default address space
git bisect good 32b994bca66641cdac8586f25315daf349921ebc
# bad: [b3052013b43617defd777b1f3b4339540c4c07df] [RISCV] Optimize (add (mul x, c0), c1)
git bisect bad b3052013b43617defd777b1f3b4339540c4c07df
# first bad commit: [b3052013b43617defd777b1f3b4339540c4c07df] [RISCV] Optimize (add (mul x, c0), c1)

$ cat evergreen.i
int evergreen_dram_bandwidth_for_display_wm_0,
    evergreen_dram_bandwidth_for_display_disp_dram_allocation_0,
    evergreen_dram_bandwidth_for_display_bandwidth_0,
    evergreen_dram_bandwidth_for_display_a_0;
void evergreen_dram_bandwidth_for_display() {
  evergreen_dram_bandwidth_for_display_a_0 = 1000 << 12;
  evergreen_dram_bandwidth_for_display_disp_dram_allocation_0 = 3 << 12;
  long tmp = evergreen_dram_bandwidth_for_display_disp_dram_allocation_0 << 13;
  tmp = tmp / evergreen_dram_bandwidth_for_display_a_0;
  evergreen_dram_bandwidth_for_display_bandwidth_0 =
      evergreen_dram_bandwidth_for_display_wm_0 * tmp + 2048;
}

$ timeout 10s clang -O2 --target=riscv64-linux-gnu -c -o /dev/null evergreen.i

$ echo $?
124

This is still reproducible at current ToT (3778c1cd6ef5a3286d5d49e842a2c65fffb8f3a6).

Thanks for the report. Hopefully https://reviews.llvm.org/rG40b230f6856d fixes this

[nathanchance]

Thanks, I can confirm that commit fixes it!

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

static SDValue combineSelectAndUseCommutative(SDNode *N, SelectionDAG &DAG,
   SDValue N1 = N->getOperand(1);
   if (SDValue Result = combineSelectAndUse(N, N0, N1, DAG, AllOnes))
     return Result;
   if (SDValue Result = combineSelectAndUse(N, N1, N0, DAG, AllOnes))
     return Result;
   return SDValue();
 }
 
+// Transform (add (mul x, c0), c1) ->
+//           (add (mul (add x, c1/c0), c0), c1%c0).
+// if c1/c0 and c1%c0 are simm12, while c1 is not.
+// Or transform (add (mul x, c0), c1) ->
+//              (mul (add x, c1/c0), c0).
+// if c1%c0 is zero, and c1/c0 is simm12 while c1 is not.
+static SDValue transformAddImmMulImm(SDNode *N, SelectionDAG &DAG,
+                                     unsigned XLen) {
+  // The type must be an integral scalar type, and does not exceeds XLen.
+  EVT VT = N->getValueType(0);
+  if (!VT.isInteger() || VT.isVector() || VT.getSizeInBits() > XLen)
+    return SDValue();
+  // The first operand node must be a MUL and has no other use.
+  SDValue N0 = N->getOperand(0);
+  if (!N0->hasOneUse() || N0->getOpcode() != ISD::MUL)
+    return SDValue();
+  // Check if c0 and c1 match above conditions.
+  auto *NC0 = dyn_cast<ConstantSDNode>(N0->getOperand(1));
+  auto *NC1 = dyn_cast<ConstantSDNode>(N->getOperand(1));
+  if (!NC0 || !NC1)
+    return SDValue();
+  int64_t C0 = NC0->getSExtValue();
+  int64_t C1 = NC1->getSExtValue();
+  if (APInt(64, C1).isSignedIntN(12) || !APInt(64, C1 % C0).isSignedIntN(12) ||
+      !APInt(64, C1 / C0).isSignedIntN(12))
+    return SDValue();
+  // Build new nodes (add (mul (add x, c1/c0), c0), c1%c0).
+  SDLoc DL(N);
+  SDValue NewOp0 = DAG.getNode(ISD::ADD, DL, VT, N0->getOperand(0),
+                               DAG.getConstant(C1 / C0, DL, VT));
+  SDValue NewOp1 = DAG.getNode(ISD::MUL, DL, VT, NewOp0,
+                               DAG.getConstant(C0, DL, VT));
+  if ((C1 % C0) == 0)
+    return NewOp1;
+  else
+    return DAG.getNode(ISD::ADD, DL, VT, NewOp1,
+                       DAG.getConstant(C1 % C0, DL, VT));
+}
+
 static SDValue performADDCombine(SDNode *N, SelectionDAG &DAG) {
   // fold (add (select lhs, rhs, cc, 0, y), x) ->
   //      (select lhs, rhs, cc, x, (add x, y))
   return combineSelectAndUseCommutative(N, DAG, /*AllOnes*/ false);
 }
 
 static SDValue performSUBCombine(SDNode *N, SelectionDAG &DAG) {
   // fold (sub x, (select lhs, rhs, cc, 0, y)) ->
   //      (select lhs, rhs, cc, x, (sub x, y))
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   return combineSelectAndUse(N, N1, N0, DAG, /*AllOnes*/ false);
 }
 
 static SDValue performANDCombine(SDNode *N, SelectionDAG &DAG) {
   // fold (and (select lhs, rhs, cc, -1, y), x) ->
   //      (select lhs, rhs, cc, x, (and x, y))
   return combineSelectAndUseCommutative(N, DAG, /*AllOnes*/ true);
 }
 
 static SDValue performORCombine(SDNode *N, SelectionDAG &DAG,
                                 const RISCVSubtarget &Subtarget) {
   if (Subtarget.hasStdExtZbp()) {
     if (auto GREV = combineORToGREV(SDValue(N, 0), DAG, Subtarget))
       return GREV;
     if (auto GORC = combineORToGORC(SDValue(N, 0), DAG, Subtarget))
       return GORC;
static SDValue performXORCombine(SDNode *N, SelectionDAG &DAG) {
   //      (select cond, x, (xor x, y))
   return combineSelectAndUseCommutative(N, DAG, /*AllOnes*/ false);
 }
 
 // Attempt to turn ANY_EXTEND into SIGN_EXTEND if the input to the ANY_EXTEND
 // has users that require SIGN_EXTEND and the SIGN_EXTEND can be done for free
 // by an instruction like ADDW/SUBW/MULW. Without this the ANY_EXTEND would be
 // removed during type legalization leaving an ADD/SUB/MUL use that won't use
 // ADDW/SUBW/MULW.
 static SDValue performANY_EXTENDCombine(SDNode *N,
                                         TargetLowering::DAGCombinerInfo &DCI,
                                         const RISCVSubtarget &Subtarget) {
   if (!Subtarget.is64Bit())
     return SDValue();
 
   SelectionDAG &DAG = DCI.DAG;
 
case RISCVISD::FMV_X_ANYEXTW_RV64: {
     if (Op0.getOpcode() == ISD::FNEG)
       return DAG.getNode(ISD::XOR, DL, VT, NewFMV,
                          DAG.getConstant(SignBit, DL, VT));
 
     assert(Op0.getOpcode() == ISD::FABS);
     return DAG.getNode(ISD::AND, DL, VT, NewFMV,
                        DAG.getConstant(~SignBit, DL, VT));
   }
-  case ISD::ADD:
+  case ISD::ADD: {
+    SDValue NA = transformAddImmMulImm(N, DAG, Subtarget.getXLen());
+    if (NA.getNode() != nullptr)
+      return NA;
     return performADDCombine(N, DAG);
+  }
   case ISD::SUB:
     return performSUBCombine(N, DAG);
   case ISD::AND:
     return performANDCombine(N, DAG);
   case ISD::OR:
     return performORCombine(N, DAG, Subtarget);
   case ISD::XOR:
     return performXORCombine(N, DAG);

llvm/test/CodeGen/RISCV/addimm-mulimm.ll

; RV64IMB-NEXT:    ret
   %tmp0 = add i64 %x, 1000
   %tmp1 = mul i64 %tmp0, 192
   ret i64 %tmp1
 }
 
 define i32 @add_mul_combine_reject_e1(i32 %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_e1:
 ; RV32IMB:       # %bb.0:
+; RV32IMB-NEXT:    addi a0, a0, 1971
 ; RV32IMB-NEXT:    addi a1, zero, 29
 ; RV32IMB-NEXT:    mul a0, a0, a1
-; RV32IMB-NEXT:    lui a1, 14
-; RV32IMB-NEXT:    addi a1, a1, -185
-; RV32IMB-NEXT:    add a0, a0, a1
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_e1:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addiw a0, a0, 1971
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mulw a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -185
-; RV64IMB-NEXT:    addw a0, a0, a1
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i32 %x, 29
   %tmp1 = add i32 %tmp0, 57159
   ret i32 %tmp1
 }
 
 define signext i32 @add_mul_combine_reject_e2(i32 signext %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_e2:
 ; RV32IMB:       # %bb.0:
+; RV32IMB-NEXT:    addi a0, a0, 1971
 ; RV32IMB-NEXT:    addi a1, zero, 29
 ; RV32IMB-NEXT:    mul a0, a0, a1
-; RV32IMB-NEXT:    lui a1, 14
-; RV32IMB-NEXT:    addi a1, a1, -185
-; RV32IMB-NEXT:    add a0, a0, a1
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_e2:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addiw a0, a0, 1971
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mulw a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -185
-; RV64IMB-NEXT:    addw a0, a0, a1
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i32 %x, 29
   %tmp1 = add i32 %tmp0, 57159
   ret i32 %tmp1
 }
 
 define i64 @add_mul_combine_reject_e3(i64 %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_e3:
 ; RV32IMB:       # %bb.0:
 ; RV32IMB-NEXT:    addi a2, zero, 29
 ; RV32IMB-NEXT:    mul a1, a1, a2
 ; RV32IMB-NEXT:    mulhu a3, a0, a2
 ; RV32IMB-NEXT:    add a1, a3, a1
 ; RV32IMB-NEXT:    mul a2, a0, a2
 ; RV32IMB-NEXT:    lui a0, 14
 ; RV32IMB-NEXT:    addi a0, a0, -185
 ; RV32IMB-NEXT:    add a0, a2, a0
 ; RV32IMB-NEXT:    sltu a2, a0, a2
 ; RV32IMB-NEXT:    add a1, a1, a2
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_e3:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addi a0, a0, 1971
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mul a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -185
-; RV64IMB-NEXT:    add a0, a0, a1
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i64 %x, 29
   %tmp1 = add i64 %tmp0, 57159
   ret i64 %tmp1
 }
 
 define i32 @add_mul_combine_reject_f1(i32 %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_f1:
 ; RV32IMB:       # %bb.0:
+; RV32IMB-NEXT:    addi a0, a0, 1972
 ; RV32IMB-NEXT:    addi a1, zero, 29
 ; RV32IMB-NEXT:    mul a0, a0, a1
-; RV32IMB-NEXT:    lui a1, 14
-; RV32IMB-NEXT:    addi a1, a1, -145
-; RV32IMB-NEXT:    add a0, a0, a1
+; RV32IMB-NEXT:    addi a0, a0, 11
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_f1:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addiw a0, a0, 1972
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mulw a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -145
-; RV64IMB-NEXT:    addw a0, a0, a1
+; RV64IMB-NEXT:    addiw a0, a0, 11
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i32 %x, 29
   %tmp1 = add i32 %tmp0, 57199
   ret i32 %tmp1
 }
 
 define signext i32 @add_mul_combine_reject_f2(i32 signext %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_f2:
 ; RV32IMB:       # %bb.0:
+; RV32IMB-NEXT:    addi a0, a0, 1972
 ; RV32IMB-NEXT:    addi a1, zero, 29
 ; RV32IMB-NEXT:    mul a0, a0, a1
-; RV32IMB-NEXT:    lui a1, 14
-; RV32IMB-NEXT:    addi a1, a1, -145
-; RV32IMB-NEXT:    add a0, a0, a1
+; RV32IMB-NEXT:    addi a0, a0, 11
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_f2:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addiw a0, a0, 1972
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mulw a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -145
-; RV64IMB-NEXT:    addw a0, a0, a1
+; RV64IMB-NEXT:    addiw a0, a0, 11
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i32 %x, 29
   %tmp1 = add i32 %tmp0, 57199
   ret i32 %tmp1
 }
 
 define i64 @add_mul_combine_reject_f3(i64 %x) {
 ; RV32IMB-LABEL: add_mul_combine_reject_f3:
 ; RV32IMB:       # %bb.0:
 ; RV32IMB-NEXT:    addi a2, zero, 29
 ; RV32IMB-NEXT:    mul a1, a1, a2
 ; RV32IMB-NEXT:    mulhu a3, a0, a2
 ; RV32IMB-NEXT:    add a1, a3, a1
 ; RV32IMB-NEXT:    mul a2, a0, a2
 ; RV32IMB-NEXT:    lui a0, 14
 ; RV32IMB-NEXT:    addi a0, a0, -145
 ; RV32IMB-NEXT:    add a0, a2, a0
 ; RV32IMB-NEXT:    sltu a2, a0, a2
 ; RV32IMB-NEXT:    add a1, a1, a2
 ; RV32IMB-NEXT:    ret
 ;
 ; RV64IMB-LABEL: add_mul_combine_reject_f3:
 ; RV64IMB:       # %bb.0:
+; RV64IMB-NEXT:    addi a0, a0, 1972
 ; RV64IMB-NEXT:    addi a1, zero, 29
 ; RV64IMB-NEXT:    mul a0, a0, a1
-; RV64IMB-NEXT:    lui a1, 14
-; RV64IMB-NEXT:    addiw a1, a1, -145
-; RV64IMB-NEXT:    add a0, a0, a1
+; RV64IMB-NEXT:    addi a0, a0, 11
 ; RV64IMB-NEXT:    ret
   %tmp0 = mul i64 %x, 29
   %tmp1 = add i64 %tmp0, 57199
   ret i64 %tmp1
 }