* [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
@ 2022-11-09 23:07 Philipp Tomsich
2022-11-18 21:13 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Philipp Tomsich @ 2022-11-09 23:07 UTC (permalink / raw)
To: gcc-patches
Cc: Christoph Muellner, Palmer Dabbelt, Vineet Gupta, Kito Cheng,
Jeff Law, Philipp Tomsich
Handling the register-const_int addition has very quickly escalated to
creating a full sign-extended 32bit constant and performing a
register-register for RISC-V in GCC so far, resulting in sequences like
(for the case of "a + 2048"):
li a5,4096
addi a5,a5,-2048
add a0,a0,a5
By adding an expansion for add<mode>3, we can emit optimised RTL that
matches the capabilities of RISC-V better by adding support for the
following, previously unoptimised cases:
- addi + addi
addi a0,a0,2047
addi a0,a0,1
- li + sh[123]add (if Zba is enabled)
li a5,960
sh3add a0,a5,a0
With this commit, we also fix up riscv_adjust_libcall_cfi_prologue()
and riscv_adjust_libcall_cfi_epilogue() to not use gen_add3_insn, as
the expander will otherwise wrap the resulting set-expression in an
insn (causing an ICE at dwarf2-time) when invoked with -msave-restore.
This closes the gap to LLVM, which has already been emitting these
optimised sequences.
Note that this benefits is perlbench (in SPEC CPU 2017), which needs
to add the constant 3840.
gcc/ChangeLog:
* config/riscv/bitmanip.md (*shNadd): Rename.
(riscv_shNadd<X:mode>): Expose as gen_riscv_shNadd{di/si}.
* config/riscv/predicates.md (const_arith_shifted123_operand):
New predicate (for constants that are a simm12, shifted by
1, 2 or 3).
(const_arith_2simm12_operand): New predicate (that can be
expressed by adding 2 simm12 together).
(addi_operand): New predicate (an immedaite operand suitable
for the new add<mode>3 expansion).
* config/riscv/riscv.cc (riscv_adjust_libcall_cfi_prologue):
Don't use gen_add3_insn, where a RTX instead of an INSN is
required (otherwise this will break as soon as we have a
define_expand for add<mode>3).
(riscv_adjust_libcall_cfi_epilogue): Same.
* config/riscv/riscv.md (addsi3): Rename.
(riscv_addsi3): New name for addsi3.
(adddi3): Rename.
(riscv_adddi3): New name for adddi3.
(add<mode>3): New expander that handles the basic and fancy
(such as li+sh[123]add, addi+addi, ...) cases for adding
register-register and register-const_int.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/addi.c: New test.
* gcc.target/riscv/zba-shNadd-06.c: New test.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
gcc/config/riscv/bitmanip.md | 2 +-
gcc/config/riscv/predicates.md | 28 +++++++++
gcc/config/riscv/riscv.cc | 10 ++--
gcc/config/riscv/riscv.md | 58 ++++++++++++++++++-
gcc/testsuite/gcc.target/riscv/addi.c | 39 +++++++++++++
.../gcc.target/riscv/zba-shNadd-06.c | 11 ++++
6 files changed, 141 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/addi.c
create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index bb23ceb86d9..78fdf02c2ec 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -29,7 +29,7 @@
[(set_attr "type" "bitmanip,load")
(set_attr "mode" "DI")])
-(define_insn "*shNadd"
+(define_insn "riscv_shNadd<X:mode>"
[(set (match_operand:X 0 "register_operand" "=r")
(plus:X (ashift:X (match_operand:X 1 "register_operand" "r")
(match_operand:QI 2 "imm123_operand" "Ds3"))
diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
index 6772228e5b6..c56bfa99339 100644
--- a/gcc/config/riscv/predicates.md
+++ b/gcc/config/riscv/predicates.md
@@ -308,3 +308,31 @@
(match_test "INTVAL (op) > 0")))
(ior (match_test "SMALL_OPERAND (UINTVAL (op) & ~(HOST_WIDE_INT_1U << floor_log2 (UINTVAL (op))))")
(match_test "popcount_hwi (UINTVAL (op)) == 2"))))
+
+;; A CONST_INT that can be shifted down by 1, 2 or 3 bits (i.e., has
+;; these bits clear) and will then form a SMALL_OPERAND.
+(define_predicate "const_arith_shifted123_operand"
+ (and (match_code "const_int")
+ (not (match_test "SMALL_OPERAND (INTVAL (op))")))
+{
+ HOST_WIDE_INT val = INTVAL (op);
+ int trailing = ctz_hwi (val);
+
+ /* Clamp to 3, as we have sh[123]add instructions only. */
+ if (trailing > 3)
+ trailing = 3;
+
+ return trailing > 0 && SMALL_OPERAND (val >> trailing);
+})
+
+;; A CONST_INT that can formed by adding two SMALL_OPERANDs together
+(define_predicate "const_arith_2simm12_operand"
+ (and (match_code "const_int")
+ (ior (match_test "SMALL_OPERAND(INTVAL (op) - ~(HOST_WIDE_INT_M1U << (IMM_BITS - 1)))")
+ (match_test "SMALL_OPERAND(INTVAL (op) - (HOST_WIDE_INT_M1U << (IMM_BITS - 1)))"))))
+
+(define_predicate "addi_operand"
+ (ior (match_operand 0 "arith_operand")
+ (match_operand 0 "const_arith_2simm12_operand")
+ (and (match_operand 0 "const_arith_shifted123_operand")
+ (match_test "TARGET_ZBA"))))
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 5a632058003..8efe90cc0fa 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4910,8 +4910,9 @@ riscv_adjust_libcall_cfi_prologue ()
}
/* Debug info for adjust sp. */
- adjust_sp_rtx = gen_add3_insn (stack_pointer_rtx,
- stack_pointer_rtx, GEN_INT (-saved_size));
+ adjust_sp_rtx = gen_rtx_SET (stack_pointer_rtx,
+ gen_rtx_PLUS (Pmode, stack_pointer_rtx,
+ GEN_INT (-saved_size)));
dwarf = alloc_reg_note (REG_CFA_ADJUST_CFA, adjust_sp_rtx,
dwarf);
return dwarf;
@@ -5013,8 +5014,9 @@ riscv_adjust_libcall_cfi_epilogue ()
int saved_size = cfun->machine->frame.save_libcall_adjustment;
/* Debug info for adjust sp. */
- adjust_sp_rtx = gen_add3_insn (stack_pointer_rtx,
- stack_pointer_rtx, GEN_INT (saved_size));
+ adjust_sp_rtx = gen_rtx_SET (stack_pointer_rtx,
+ gen_rtx_PLUS (Pmode, stack_pointer_rtx,
+ GEN_INT (saved_size)));
dwarf = alloc_reg_note (REG_CFA_ADJUST_CFA, adjust_sp_rtx,
dwarf);
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 171a0cdced6..289ff7470c6 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -446,7 +446,7 @@
[(set_attr "type" "fadd")
(set_attr "mode" "<UNITMODE>")])
-(define_insn "addsi3"
+(define_insn "riscv_addsi3"
[(set (match_operand:SI 0 "register_operand" "=r,r")
(plus:SI (match_operand:SI 1 "register_operand" " r,r")
(match_operand:SI 2 "arith_operand" " r,I")))]
@@ -455,7 +455,7 @@
[(set_attr "type" "arith")
(set_attr "mode" "SI")])
-(define_insn "adddi3"
+(define_insn "riscv_adddi3"
[(set (match_operand:DI 0 "register_operand" "=r,r")
(plus:DI (match_operand:DI 1 "register_operand" " r,r")
(match_operand:DI 2 "arith_operand" " r,I")))]
@@ -464,6 +464,60 @@
[(set_attr "type" "arith")
(set_attr "mode" "DI")])
+(define_expand "add<mode>3"
+ [(set (match_operand:GPR 0 "register_operand" "=r,r")
+ (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
+ (match_operand:GPR 2 "addi_operand" " r,I")))]
+ ""
+{
+ if (arith_operand (operands[2], <MODE>mode))
+ emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
+ else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
+ {
+ /* Split into two immediates that add up to the desired value:
+ * e.g., break up "a + 2445" into:
+ * addi a0,a0,2047
+ * addi a0,a0,398
+ */
+
+ HOST_WIDE_INT val = INTVAL (operands[2]);
+ HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
+
+ if (val >= 0)
+ saturated = ~saturated;
+
+ val -= saturated;
+
+ rtx tmp = gen_reg_rtx (<MODE>mode);
+ emit_insn (gen_riscv_add<mode>3 (tmp, operands[1], GEN_INT (saturated)));
+ emit_insn (gen_riscv_add<mode>3 (operands[0], tmp, GEN_INT (val)));
+ }
+ else if (<MODE>mode == word_mode
+ && const_arith_shifted123_operand (operands[2], <MODE>mode))
+ {
+ /* Use a sh[123]add and an immediate shifted down by 1, 2, or 3. */
+
+ HOST_WIDE_INT val = INTVAL (operands[2]);
+ int shamt = ctz_hwi (val);
+
+ if (shamt > 3)
+ shamt = 3;
+
+ rtx tmp = gen_reg_rtx (<MODE>mode);
+ emit_insn (gen_rtx_SET (tmp, GEN_INT (val >> shamt)));
+
+ /* We don't use gen_riscv_shNadd here, as it will only exist for
+ <X:mode>. Instead we build up its canonical form directly. */
+ rtx shifted_imm = gen_rtx_ASHIFT (<MODE>mode, tmp, GEN_INT (shamt));
+ rtx shNadd = gen_rtx_PLUS (<MODE>mode, shifted_imm, operands[1]);
+ emit_insn (gen_rtx_SET (operands[0], shNadd));
+ }
+ else
+ FAIL;
+
+ DONE;
+})
+
(define_expand "addv<mode>4"
[(set (match_operand:GPR 0 "register_operand" "=r,r")
(plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
diff --git a/gcc/testsuite/gcc.target/riscv/addi.c b/gcc/testsuite/gcc.target/riscv/addi.c
new file mode 100644
index 00000000000..01339e44697
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/addi.c
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64" } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+
+long long f (long long a)
+{
+ // addi a0,a0,2047
+ // addi a0,a0,1
+
+ return a + 2048;
+}
+
+long long f2 (long long a)
+{
+ // addi a0,a0,2047
+ // addi a0,a0,398
+
+ return a + 2445;
+}
+
+long long f3 (long long a)
+{
+ // addi a0,a0,-2048
+ // addi a0,a0,-397
+
+ return a - 2445;
+}
+
+long long f6 (long long a)
+{
+ // li a5,1179648
+ // add a0,a0,a5
+
+ return a + (0x12 << 16);
+}
+
+/* { dg-final { scan-assembler-times "addi\t" 6 } } */
+/* { dg-final { scan-assembler-times "li\t" 1 } } */
+/* { dg-final { scan-assembler-times "add\t" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c b/gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
new file mode 100644
index 00000000000..c55f05ed1d2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zba -mabi=lp64" } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+
+long long f (long long a)
+{
+ return a + 7680;
+}
+
+/* { dg-final { scan-assembler-times "li\t" 1 } } */
+/* { dg-final { scan-assembler-times "sh3add\t" 1 } } */
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
2022-11-09 23:07 [PATCH] RISC-V: Optimise adding a (larger than simm12) constant Philipp Tomsich
@ 2022-11-18 21:13 ` Jeff Law
2022-11-18 21:26 ` Philipp Tomsich
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2022-11-18 21:13 UTC (permalink / raw)
To: Philipp Tomsich, gcc-patches
Cc: Christoph Muellner, Palmer Dabbelt, Vineet Gupta, Kito Cheng, Jeff Law
On 11/9/22 16:07, Philipp Tomsich wrote:
> Handling the register-const_int addition has very quickly escalated to
> creating a full sign-extended 32bit constant and performing a
> register-register for RISC-V in GCC so far, resulting in sequences like
> (for the case of "a + 2048"):
> li a5,4096
> addi a5,a5,-2048
> add a0,a0,a5
>
> By adding an expansion for add<mode>3, we can emit optimised RTL that
> matches the capabilities of RISC-V better by adding support for the
> following, previously unoptimised cases:
> - addi + addi
> addi a0,a0,2047
> addi a0,a0,1
> - li + sh[123]add (if Zba is enabled)
> li a5,960
> sh3add a0,a5,a0
>
> With this commit, we also fix up riscv_adjust_libcall_cfi_prologue()
> and riscv_adjust_libcall_cfi_epilogue() to not use gen_add3_insn, as
> the expander will otherwise wrap the resulting set-expression in an
> insn (causing an ICE at dwarf2-time) when invoked with -msave-restore.
>
> This closes the gap to LLVM, which has already been emitting these
> optimised sequences.
>
> Note that this benefits is perlbench (in SPEC CPU 2017), which needs
> to add the constant 3840.
>
> gcc/ChangeLog:
>
> * config/riscv/bitmanip.md (*shNadd): Rename.
> (riscv_shNadd<X:mode>): Expose as gen_riscv_shNadd{di/si}.
> * config/riscv/predicates.md (const_arith_shifted123_operand):
> New predicate (for constants that are a simm12, shifted by
> 1, 2 or 3).
> (const_arith_2simm12_operand): New predicate (that can be
> expressed by adding 2 simm12 together).
> (addi_operand): New predicate (an immedaite operand suitable
> for the new add<mode>3 expansion).
> * config/riscv/riscv.cc (riscv_adjust_libcall_cfi_prologue):
> Don't use gen_add3_insn, where a RTX instead of an INSN is
> required (otherwise this will break as soon as we have a
> define_expand for add<mode>3).
> (riscv_adjust_libcall_cfi_epilogue): Same.
> * config/riscv/riscv.md (addsi3): Rename.
> (riscv_addsi3): New name for addsi3.
> (adddi3): Rename.
> (riscv_adddi3): New name for adddi3.
> (add<mode>3): New expander that handles the basic and fancy
> (such as li+sh[123]add, addi+addi, ...) cases for adding
> register-register and register-const_int.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/addi.c: New test.
> * gcc.target/riscv/zba-shNadd-06.c: New test.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
> gcc/config/riscv/bitmanip.md | 2 +-
> gcc/config/riscv/predicates.md | 28 +++++++++
> gcc/config/riscv/riscv.cc | 10 ++--
> gcc/config/riscv/riscv.md | 58 ++++++++++++++++++-
> gcc/testsuite/gcc.target/riscv/addi.c | 39 +++++++++++++
> .../gcc.target/riscv/zba-shNadd-06.c | 11 ++++
> 6 files changed, 141 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/addi.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
>
>
>
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 171a0cdced6..289ff7470c6 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -464,6 +464,60 @@
> [(set_attr "type" "arith")
> (set_attr "mode" "DI")])
>
> +(define_expand "add<mode>3"
> + [(set (match_operand:GPR 0 "register_operand" "=r,r")
> + (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
> + (match_operand:GPR 2 "addi_operand" " r,I")))]
> + ""
> +{
> + if (arith_operand (operands[2], <MODE>mode))
> + emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
> + else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
> + {
> + /* Split into two immediates that add up to the desired value:
> + * e.g., break up "a + 2445" into:
> + * addi a0,a0,2047
> + * addi a0,a0,398
> + */
Nit. GNU comment style please.
> +
> + HOST_WIDE_INT val = INTVAL (operands[2]);
> + HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
> +
> + if (val >= 0)
> + saturated = ~saturated;
> +
> + val -= saturated;
> +
> + rtx tmp = gen_reg_rtx (<MODE>mode);
Can't add<mode>3 be generated by LRA? If so, don't you have to guard
against going into this path as we shouldn't be creating new pseudos at
that point (I know LRA can create some internally, but I don't think it
handles new ones showing up due to target expanders).
Similarly for the shifted_123 case immediately following.
If we do indeed have an issue here, I'm not sure how best to resolve.
If the output operand does not overlap with the inputs, then we're
golden and can just re-use it to form the constant. If not, then it's
a bit tougher. I'm not keen to add a test of no_new_pseudos to the
operand predicate, but I don't see a better option yet.
jeff
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
2022-11-18 21:13 ` Jeff Law
@ 2022-11-18 21:26 ` Philipp Tomsich
2022-11-18 22:14 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: Philipp Tomsich @ 2022-11-18 21:26 UTC (permalink / raw)
To: Jeff Law
Cc: gcc-patches, Christoph Muellner, Palmer Dabbelt, Vineet Gupta,
Kito Cheng, Jeff Law
On Fri, 18 Nov 2022 at 22:13, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 11/9/22 16:07, Philipp Tomsich wrote:
> > Handling the register-const_int addition has very quickly escalated to
> > creating a full sign-extended 32bit constant and performing a
> > register-register for RISC-V in GCC so far, resulting in sequences like
> > (for the case of "a + 2048"):
> > li a5,4096
> > addi a5,a5,-2048
> > add a0,a0,a5
> >
> > By adding an expansion for add<mode>3, we can emit optimised RTL that
> > matches the capabilities of RISC-V better by adding support for the
> > following, previously unoptimised cases:
> > - addi + addi
> > addi a0,a0,2047
> > addi a0,a0,1
> > - li + sh[123]add (if Zba is enabled)
> > li a5,960
> > sh3add a0,a5,a0
> >
> > With this commit, we also fix up riscv_adjust_libcall_cfi_prologue()
> > and riscv_adjust_libcall_cfi_epilogue() to not use gen_add3_insn, as
> > the expander will otherwise wrap the resulting set-expression in an
> > insn (causing an ICE at dwarf2-time) when invoked with -msave-restore.
> >
> > This closes the gap to LLVM, which has already been emitting these
> > optimised sequences.
> >
> > Note that this benefits is perlbench (in SPEC CPU 2017), which needs
> > to add the constant 3840.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/bitmanip.md (*shNadd): Rename.
> > (riscv_shNadd<X:mode>): Expose as gen_riscv_shNadd{di/si}.
> > * config/riscv/predicates.md (const_arith_shifted123_operand):
> > New predicate (for constants that are a simm12, shifted by
> > 1, 2 or 3).
> > (const_arith_2simm12_operand): New predicate (that can be
> > expressed by adding 2 simm12 together).
> > (addi_operand): New predicate (an immedaite operand suitable
> > for the new add<mode>3 expansion).
> > * config/riscv/riscv.cc (riscv_adjust_libcall_cfi_prologue):
> > Don't use gen_add3_insn, where a RTX instead of an INSN is
> > required (otherwise this will break as soon as we have a
> > define_expand for add<mode>3).
> > (riscv_adjust_libcall_cfi_epilogue): Same.
> > * config/riscv/riscv.md (addsi3): Rename.
> > (riscv_addsi3): New name for addsi3.
> > (adddi3): Rename.
> > (riscv_adddi3): New name for adddi3.
> > (add<mode>3): New expander that handles the basic and fancy
> > (such as li+sh[123]add, addi+addi, ...) cases for adding
> > register-register and register-const_int.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/addi.c: New test.
> > * gcc.target/riscv/zba-shNadd-06.c: New test.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> > gcc/config/riscv/bitmanip.md | 2 +-
> > gcc/config/riscv/predicates.md | 28 +++++++++
> > gcc/config/riscv/riscv.cc | 10 ++--
> > gcc/config/riscv/riscv.md | 58 ++++++++++++++++++-
> > gcc/testsuite/gcc.target/riscv/addi.c | 39 +++++++++++++
> > .../gcc.target/riscv/zba-shNadd-06.c | 11 ++++
> > 6 files changed, 141 insertions(+), 7 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/riscv/addi.c
> > create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
> >
> >
> >
> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> > index 171a0cdced6..289ff7470c6 100644
> > --- a/gcc/config/riscv/riscv.md
> > +++ b/gcc/config/riscv/riscv.md
> > @@ -464,6 +464,60 @@
> > [(set_attr "type" "arith")
> > (set_attr "mode" "DI")])
> >
> > +(define_expand "add<mode>3"
> > + [(set (match_operand:GPR 0 "register_operand" "=r,r")
> > + (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
> > + (match_operand:GPR 2 "addi_operand" " r,I")))]
> > + ""
> > +{
> > + if (arith_operand (operands[2], <MODE>mode))
> > + emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
> > + else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
> > + {
> > + /* Split into two immediates that add up to the desired value:
> > + * e.g., break up "a + 2445" into:
> > + * addi a0,a0,2047
> > + * addi a0,a0,398
> > + */
>
> Nit. GNU comment style please.
>
>
> > +
> > + HOST_WIDE_INT val = INTVAL (operands[2]);
> > + HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
> > +
> > + if (val >= 0)
> > + saturated = ~saturated;
> > +
> > + val -= saturated;
> > +
> > + rtx tmp = gen_reg_rtx (<MODE>mode);
>
> Can't add<mode>3 be generated by LRA? If so, don't you have to guard
> against going into this path as we shouldn't be creating new pseudos at
> that point (I know LRA can create some internally, but I don't think it
> handles new ones showing up due to target expanders).
>
>
> Similarly for the shifted_123 case immediately following.
>
>
> If we do indeed have an issue here, I'm not sure how best to resolve.
> If the output operand does not overlap with the inputs, then we're
> golden and can just re-use it to form the constant. If not, then it's
> a bit tougher. I'm not keen to add a test of no_new_pseudos to the
> operand predicate, but I don't see a better option yet.
From a cursory glance, LRA does not try to go through gen_add3_insn,
but rather forms PLUS rtx. This matches my expectation, as (if we
can't create a pseudo) the original add<mode>3 definition would run
into the same problem (as an immediate that doesn't satisfy the
SMALL_OPERAND test will need to be placed in a register).
Philipp.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
2022-11-18 21:26 ` Philipp Tomsich
@ 2022-11-18 22:14 ` Jeff Law
2022-11-21 3:10 ` Kito Cheng
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2022-11-18 22:14 UTC (permalink / raw)
To: Philipp Tomsich, Jeff Law
Cc: gcc-patches, Christoph Muellner, Palmer Dabbelt, Vineet Gupta,
Kito Cheng
On 11/18/22 14:26, Philipp Tomsich wrote:
> On Fri, 18 Nov 2022 at 22:13, Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>> On 11/9/22 16:07, Philipp Tomsich wrote:
>>> Handling the register-const_int addition has very quickly escalated to
>>> creating a full sign-extended 32bit constant and performing a
>>> register-register for RISC-V in GCC so far, resulting in sequences like
>>> (for the case of "a + 2048"):
>>> li a5,4096
>>> addi a5,a5,-2048
>>> add a0,a0,a5
>>>
>>> By adding an expansion for add<mode>3, we can emit optimised RTL that
>>> matches the capabilities of RISC-V better by adding support for the
>>> following, previously unoptimised cases:
>>> - addi + addi
>>> addi a0,a0,2047
>>> addi a0,a0,1
>>> - li + sh[123]add (if Zba is enabled)
>>> li a5,960
>>> sh3add a0,a5,a0
>>>
>>> With this commit, we also fix up riscv_adjust_libcall_cfi_prologue()
>>> and riscv_adjust_libcall_cfi_epilogue() to not use gen_add3_insn, as
>>> the expander will otherwise wrap the resulting set-expression in an
>>> insn (causing an ICE at dwarf2-time) when invoked with -msave-restore.
>>>
>>> This closes the gap to LLVM, which has already been emitting these
>>> optimised sequences.
>>>
>>> Note that this benefits is perlbench (in SPEC CPU 2017), which needs
>>> to add the constant 3840.
>>>
>>> gcc/ChangeLog:
>>>
>>> * config/riscv/bitmanip.md (*shNadd): Rename.
>>> (riscv_shNadd<X:mode>): Expose as gen_riscv_shNadd{di/si}.
>>> * config/riscv/predicates.md (const_arith_shifted123_operand):
>>> New predicate (for constants that are a simm12, shifted by
>>> 1, 2 or 3).
>>> (const_arith_2simm12_operand): New predicate (that can be
>>> expressed by adding 2 simm12 together).
>>> (addi_operand): New predicate (an immedaite operand suitable
>>> for the new add<mode>3 expansion).
>>> * config/riscv/riscv.cc (riscv_adjust_libcall_cfi_prologue):
>>> Don't use gen_add3_insn, where a RTX instead of an INSN is
>>> required (otherwise this will break as soon as we have a
>>> define_expand for add<mode>3).
>>> (riscv_adjust_libcall_cfi_epilogue): Same.
>>> * config/riscv/riscv.md (addsi3): Rename.
>>> (riscv_addsi3): New name for addsi3.
>>> (adddi3): Rename.
>>> (riscv_adddi3): New name for adddi3.
>>> (add<mode>3): New expander that handles the basic and fancy
>>> (such as li+sh[123]add, addi+addi, ...) cases for adding
>>> register-register and register-const_int.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * gcc.target/riscv/addi.c: New test.
>>> * gcc.target/riscv/zba-shNadd-06.c: New test.
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>>> ---
>>>
>>> gcc/config/riscv/bitmanip.md | 2 +-
>>> gcc/config/riscv/predicates.md | 28 +++++++++
>>> gcc/config/riscv/riscv.cc | 10 ++--
>>> gcc/config/riscv/riscv.md | 58 ++++++++++++++++++-
>>> gcc/testsuite/gcc.target/riscv/addi.c | 39 +++++++++++++
>>> .../gcc.target/riscv/zba-shNadd-06.c | 11 ++++
>>> 6 files changed, 141 insertions(+), 7 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.target/riscv/addi.c
>>> create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-06.c
>>>
>>>
>>>
>>> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
>>> index 171a0cdced6..289ff7470c6 100644
>>> --- a/gcc/config/riscv/riscv.md
>>> +++ b/gcc/config/riscv/riscv.md
>>> @@ -464,6 +464,60 @@
>>> [(set_attr "type" "arith")
>>> (set_attr "mode" "DI")])
>>>
>>> +(define_expand "add<mode>3"
>>> + [(set (match_operand:GPR 0 "register_operand" "=r,r")
>>> + (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
>>> + (match_operand:GPR 2 "addi_operand" " r,I")))]
>>> + ""
>>> +{
>>> + if (arith_operand (operands[2], <MODE>mode))
>>> + emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
>>> + else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
>>> + {
>>> + /* Split into two immediates that add up to the desired value:
>>> + * e.g., break up "a + 2445" into:
>>> + * addi a0,a0,2047
>>> + * addi a0,a0,398
>>> + */
>> Nit. GNU comment style please.
>>
>>
>>> +
>>> + HOST_WIDE_INT val = INTVAL (operands[2]);
>>> + HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
>>> +
>>> + if (val >= 0)
>>> + saturated = ~saturated;
>>> +
>>> + val -= saturated;
>>> +
>>> + rtx tmp = gen_reg_rtx (<MODE>mode);
>> Can't add<mode>3 be generated by LRA? If so, don't you have to guard
>> against going into this path as we shouldn't be creating new pseudos at
>> that point (I know LRA can create some internally, but I don't think it
>> handles new ones showing up due to target expanders).
>>
>>
>> Similarly for the shifted_123 case immediately following.
>>
>>
>> If we do indeed have an issue here, I'm not sure how best to resolve.
>> If the output operand does not overlap with the inputs, then we're
>> golden and can just re-use it to form the constant. If not, then it's
>> a bit tougher. I'm not keen to add a test of no_new_pseudos to the
>> operand predicate, but I don't see a better option yet.
> From a cursory glance, LRA does not try to go through gen_add3_insn,
> but rather forms PLUS rtx. This matches my expectation, as (if we
> can't create a pseudo) the original add<mode>3 definition would run
> into the same problem (as an immediate that doesn't satisfy the
> SMALL_OPERAND test will need to be placed in a register).
I should have remembered that. It vectors through emit_add3_insn which
you and I have looked at it not terribly long ago. We don't define
addptr3, so it'll just form the expression directly rather than going
through the expanders.
So, OK.
jeff
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
2022-11-18 22:14 ` Jeff Law
@ 2022-11-21 3:10 ` Kito Cheng
2022-11-21 10:13 ` Philipp Tomsich
0 siblings, 1 reply; 6+ messages in thread
From: Kito Cheng @ 2022-11-21 3:10 UTC (permalink / raw)
To: Jeff Law
Cc: Philipp Tomsich, Jeff Law, gcc-patches, Christoph Muellner,
Palmer Dabbelt, Vineet Gupta
> @@ -464,6 +464,60 @@
> [(set_attr "type" "arith")
> (set_attr "mode" "DI")])
>
> +(define_expand "add<mode>3"
> + [(set (match_operand:GPR 0 "register_operand" "=r,r")
> + (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
> + (match_operand:GPR 2 "addi_operand" " r,I")))]
Is it possible to just define a predicate that accepts
register_operand and CONST_INT_P,
and then handle all cases in add<mode>3 pattern?
My point is put all check in one place:
e.g.
check TARGET_ZBA && const_arith_shifted123_operand (operands[2],
<MODE>mode) in add<mode>3
rather than check TARGET_ZBA in addi_operand and use sh[123]add in
add<mode>3 without check.
and that also means we need to sync addi_opearnad and add<mode>3
once we have extension XX could improve addi codegen.
> + ""
> +{
> + if (arith_operand (operands[2], <MODE>mode))
> + emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
> + else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
const_arith_2simm12_operand only used once, could you inline the condition here?
> + {
> + /* Split into two immediates that add up to the desired value:
> + * e.g., break up "a + 2445" into:
> + * addi a0,a0,2047
> + * addi a0,a0,398
> + */
> +
> + HOST_WIDE_INT val = INTVAL (operands[2]);
> + HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
> +
> + if (val >= 0)
> + saturated = ~saturated;
> +
> + val -= saturated;
> +
> + rtx tmp = gen_reg_rtx (<MODE>mode);
> + emit_insn (gen_riscv_add<mode>3 (tmp, operands[1], GEN_INT (saturated)));
> + emit_insn (gen_riscv_add<mode>3 (operands[0], tmp, GEN_INT (val)));
> + }
> + else if (<MODE>mode == word_mode
> + && const_arith_shifted123_operand (operands[2], <MODE>mode))
Same for const_arith_shifted123_operand.
> + {
> + /* Use a sh[123]add and an immediate shifted down by 1, 2, or 3. */
> +
> + HOST_WIDE_INT val = INTVAL (operands[2]);
> + int shamt = ctz_hwi (val);
> +
> + if (shamt > 3)
> + shamt = 3;
> +
> + rtx tmp = gen_reg_rtx (<MODE>mode);
> + emit_insn (gen_rtx_SET (tmp, GEN_INT (val >> shamt)));
> +
> + /* We don't use gen_riscv_shNadd here, as it will only exist for
> + <X:mode>. Instead we build up its canonical form directly. */
> + rtx shifted_imm = gen_rtx_ASHIFT (<MODE>mode, tmp, GEN_INT (shamt));
> + rtx shNadd = gen_rtx_PLUS (<MODE>mode, shifted_imm, operands[1]);
> + emit_insn (gen_rtx_SET (operands[0], shNadd));
> + }
> + else
> + FAIL;
Seems add<mode>3 FAIL will cause problems, we need either add something like:
operands[2] = force_reg (<MODE>mode, operands[2]);
emit_insn (gen_rtx_SET (operands[0],
gen_rtx_PLUS (<MODE>mode,
operands[1], operands[2])));
Or just gcc_unreachable () if we keep using addi_operand to guard this pattern.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: Optimise adding a (larger than simm12) constant
2022-11-21 3:10 ` Kito Cheng
@ 2022-11-21 10:13 ` Philipp Tomsich
0 siblings, 0 replies; 6+ messages in thread
From: Philipp Tomsich @ 2022-11-21 10:13 UTC (permalink / raw)
To: Kito Cheng
Cc: Jeff Law, Jeff Law, gcc-patches, Christoph Muellner,
Palmer Dabbelt, Vineet Gupta
On Mon, 21 Nov 2022 at 04:11, Kito Cheng <kito.cheng@gmail.com> wrote:
>
> > @@ -464,6 +464,60 @@
> > [(set_attr "type" "arith")
> > (set_attr "mode" "DI")])
> >
> > +(define_expand "add<mode>3"
> > + [(set (match_operand:GPR 0 "register_operand" "=r,r")
> > + (plus:GPR (match_operand:GPR 1 "register_operand" " r,r")
> > + (match_operand:GPR 2 "addi_operand" " r,I")))]
>
> Is it possible to just define a predicate that accepts
> register_operand and CONST_INT_P,
> and then handle all cases in add<mode>3 pattern?
Great suggestion.
>
> My point is put all check in one place:
>
> e.g.
> check TARGET_ZBA && const_arith_shifted123_operand (operands[2],
> <MODE>mode) in add<mode>3
> rather than check TARGET_ZBA in addi_operand and use sh[123]add in
> add<mode>3 without check.
>
> and that also means we need to sync addi_opearnad and add<mode>3
> once we have extension XX could improve addi codegen.
>
>
> > + ""
> > +{
> > + if (arith_operand (operands[2], <MODE>mode))
> > + emit_insn (gen_riscv_add<mode>3 (operands[0], operands[1], operands[2]));
> > + else if (const_arith_2simm12_operand (operands[2], <MODE>mode))
>
> const_arith_2simm12_operand only used once, could you inline the condition here?
If we handle all cases in a single pattern, we'll punt this to riscv.cc anyway.
So let's see how the code looks once we have a single predicate and do
the inlining there...
>
> > + {
> > + /* Split into two immediates that add up to the desired value:
> > + * e.g., break up "a + 2445" into:
> > + * addi a0,a0,2047
> > + * addi a0,a0,398
> > + */
> > +
> > + HOST_WIDE_INT val = INTVAL (operands[2]);
> > + HOST_WIDE_INT saturated = HOST_WIDE_INT_M1U << (IMM_BITS - 1);
> > +
> > + if (val >= 0)
> > + saturated = ~saturated;
> > +
> > + val -= saturated;
> > +
> > + rtx tmp = gen_reg_rtx (<MODE>mode);
> > + emit_insn (gen_riscv_add<mode>3 (tmp, operands[1], GEN_INT (saturated)));
> > + emit_insn (gen_riscv_add<mode>3 (operands[0], tmp, GEN_INT (val)));
> > + }
> > + else if (<MODE>mode == word_mode
> > + && const_arith_shifted123_operand (operands[2], <MODE>mode))
>
> Same for const_arith_shifted123_operand.
>
> > + {
> > + /* Use a sh[123]add and an immediate shifted down by 1, 2, or 3. */
> > +
> > + HOST_WIDE_INT val = INTVAL (operands[2]);
> > + int shamt = ctz_hwi (val);
> > +
> > + if (shamt > 3)
> > + shamt = 3;
> > +
> > + rtx tmp = gen_reg_rtx (<MODE>mode);
> > + emit_insn (gen_rtx_SET (tmp, GEN_INT (val >> shamt)));
> > +
> > + /* We don't use gen_riscv_shNadd here, as it will only exist for
> > + <X:mode>. Instead we build up its canonical form directly. */
> > + rtx shifted_imm = gen_rtx_ASHIFT (<MODE>mode, tmp, GEN_INT (shamt));
> > + rtx shNadd = gen_rtx_PLUS (<MODE>mode, shifted_imm, operands[1]);
> > + emit_insn (gen_rtx_SET (operands[0], shNadd));
> > + }
> > + else
> > + FAIL;
>
> Seems add<mode>3 FAIL will cause problems, we need either add something like:
>
> operands[2] = force_reg (<MODE>mode, operands[2]);
> emit_insn (gen_rtx_SET (operands[0],
> gen_rtx_PLUS (<MODE>mode,
> operands[1], operands[2])));
>
> Or just gcc_unreachable () if we keep using addi_operand to guard this pattern.
This is a case for "gcc_unreachable ();".
The change will be in v2.
Thanks,
Philipp.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-21 10:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-09 23:07 [PATCH] RISC-V: Optimise adding a (larger than simm12) constant Philipp Tomsich
2022-11-18 21:13 ` Jeff Law
2022-11-18 21:26 ` Philipp Tomsich
2022-11-18 22:14 ` Jeff Law
2022-11-21 3:10 ` Kito Cheng
2022-11-21 10:13 ` Philipp Tomsich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).