[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 ↗

(On Diff #368264)

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

5864 ↗

(On Diff #368264)

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 ↗

(On Diff #368264)

No need for else after a return.

[benshi001]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

5864 ↗

(On Diff #368264)

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

6191 ↗

(On Diff #369598)

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

5851 ↗

(On Diff #373123)

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

5859 ↗

(On Diff #373123)

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

5887 ↗

(On Diff #373123)

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/test/CodeGen/RISCV/addimm-mulimm.ll

 ; RV64IMB-NEXT:    addi a0, a0, 1000
 ; RV64IMB-NEXT:    sh1add a0, a0, a0
 ; RV64IMB-NEXT:    slli a0, a0, 6
 ; 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 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:    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 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:    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 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 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:    ret
+;
+; RV64IMB-LABEL: add_mul_combine_reject_f1:
+; RV64IMB:       # %bb.0:
+; 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:    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 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:    ret
+;
+; RV64IMB-LABEL: add_mul_combine_reject_f2:
+; RV64IMB:       # %bb.0:
+; 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:    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 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:    ret
+  %tmp0 = mul i64 %x, 29
+  %tmp1 = add i64 %tmp0, 57199
+  ret i64 %tmp1
+}