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

[Jeff Law <jeffreyalaw@gmail.com>]

On 2/23/23 14:23, Andrew Pinski via Gcc-patches wrote:
> On Fri, Dec 9, 2022 at 10:25 AM Raphael Moreira Zinsly
> <rzinsly@ventanamicro.com> wrote:
>>
>> Changes since v1:
>>          - Fixed formatting issues.
>>          - Added a name to the define_insn_and_split pattern.
>>          - Set the target on the 'dg-do compile' in pr106602.c.
>>          - Removed the rv32 restriction in pr95632.c.
>>
>> -- >8 --
>>
>> 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.
>>
>> 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: New test.
>> ---
>>   gcc/config/riscv/riscv.md                 | 15 +++++++++++++++
>>   gcc/testsuite/gcc.target/riscv/pr106602.c | 14 ++++++++++++++
>>   gcc/testsuite/gcc.target/riscv/pr95632.c  | 15 +++++++++++++++
>>   3 files changed, 44 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..b0daa4b19eb 100644
>> --- a/gcc/config/riscv/riscv.md
>> +++ b/gcc/config/riscv/riscv.md
>> @@ -1667,6 +1667,21 @@
>>                        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 "*mvconst_internal"
>> +  [(set (match_operand:GPR 0 "register_operand" "=r")
>> +    (match_operand:GPR 1 "splittable_const_int_operand" "i"))]
>> +  "cse_not_expected"
> 
> This is just way broken. This should be combined with the normal move
> instructions and just be a define_split.
> See PR 108892 for a testcase which shows this breaking how the
> register allocator thinks it should work.
I'm pretty sure that won't work.  You need them exposed as a define_insn 
so that they can act as a bridge pattern for combine.  You don't want to 
expose before combine as that'll regress things in a variety of other 
ways.  You don't want the bridge form to survive after splitting.  Hence 
define_insn_and_split.

I haven't looked at that bug in detail, but Raphael and I certainly will.

jeff