Re: [PATCH] RISC-V: Produce better code with complex constants [PR95632] [PR106602]

[Palmer Dabbelt <palmer@dabbelt.com>]

On Wed, 07 Dec 2022 12:55:17 PST (-0800), rzinsly@ventanamicro.com wrote:
> Due to RISC-V limitations on operations with big constants combine
> is failing to match such operations and is not being able to
> produce optimal code as it keeps splitting them. By pretending we
> can do those operations we can get more opportunities for
> simplification of surrounding instructions.

I saw Jeff's comments.  This is always the kind of thing that worries 
me: we're essentially lying to the optimizer in order to trick it into 
generating better code, which might just make it generate worse code.  
It's always easy to see a small example that improves, but those could 
be wiped out by secondary effects in real code.  So I'd usually want to 
have some benchmarking for a patch like this.

That said, if this is just the standard way of doing things then maybe 
it's just fine?

> 2022-12-06 Raphael Moreira Zinsly <rzinsly@ventanamicro.com>
>            Jeff Law <jlaw@ventanamicro.com>
>
> gcc/Changelog:
> 	PR target/95632
>         PR target/106602
>         * config/riscv/riscv.md: New pattern to simulate complex
>         const_int loads.
>
> gcc/testsuite/ChangeLog:
> 	* gcc.target/riscv/pr95632.c: New test.
>         * gcc.target/riscv/pr106602.c: Likewise.
> ---
>  gcc/config/riscv/riscv.md                 | 16 ++++++++++++++++
>  gcc/testsuite/gcc.target/riscv/pr106602.c | 14 ++++++++++++++
>  gcc/testsuite/gcc.target/riscv/pr95632.c  | 15 +++++++++++++++
>  3 files changed, 45 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/pr106602.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/pr95632.c
>
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index df57e2b0b4a..0a9b5ec22b0 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -1667,6 +1667,22 @@
>  		      MAX_MACHINE_MODE, &operands[3], TRUE);
>  })
>
> +;; Pretend to have the ability to load complex const_int in order to get
> +;; better code generation around them.
> +(define_insn_and_split ""
> +  [(set (match_operand:GPR 0 "register_operand" "=r")
> +    (match_operand:GPR 1 "splittable_const_int_operand" "i"))]
> +  "cse_not_expected"
> +  "#"
> +  "&& 1"
> +  [(const_int 0)]
> +
> +{
> +  riscv_move_integer (operands[0], operands[0], INTVAL (operands[1]),
> +		      <GPR:MODE>mode, TRUE);
> +  DONE;
> +})

There's some comments from Jakub on this, I don't see any additional 
issues with the code (aside from the "does it help" stuff from above).

> +
>  ;; 64-bit integer moves
>
>  (define_expand "movdi"
> diff --git a/gcc/testsuite/gcc.target/riscv/pr106602.c b/gcc/testsuite/gcc.target/riscv/pr106602.c
> new file mode 100644
> index 00000000000..83b70877012
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr106602.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=rv64gc" } */

There's a DG hook to limit this to 64-bit targets, that way it'll run 
with whatever target is being tested.

> +
> +unsigned long
> +foo2 (unsigned long a)
> +{
> +  return (unsigned long)(unsigned int) a << 6;
> +}
> +
> +/* { dg-final { scan-assembler-times "slli\t" 1 } } */
> +/* { dg-final { scan-assembler-times "srli\t" 1 } } */
> +/* { dg-final { scan-assembler-not "\tli\t" } } */
> +/* { dg-final { scan-assembler-not "addi\t" } } */
> +/* { dg-final { scan-assembler-not "and\t" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/pr95632.c b/gcc/testsuite/gcc.target/riscv/pr95632.c
> new file mode 100644
> index 00000000000..bd316ab1d7b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr95632.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=rv32imafc -mabi=ilp32f" } */

Is there a reason to make this rv32-only?  Unless I'm missing something 
this should generate pretty much the same code for rv64.

> +
> +unsigned short
> +foo (unsigned short crc)
> +{
> +  crc ^= 0x4002;
> +  crc >>= 1;
> +  crc |= 0x8000;
> +
> +  return crc;
> +}
> +
> +/* { dg-final { scan-assembler-times "srli\t" 1 } } */
> +/* { dg-final { scan-assembler-not "slli\t" } } */