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

[Palmer Dabbelt <palmer@dabbelt.com>]

On Wed, 07 Dec 2022 13:30:32 PST (-0800), gcc-patches@gcc.gnu.org wrote:
> On Wed, Dec 07, 2022 at 05:55:17PM -0300, Raphael Moreira Zinsly 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.
>>
>> 2022-12-06 Raphael Moreira Zinsly <rzinsly@ventanamicro.com>
>>            Jeff Law <jlaw@ventanamicro.com>
>
> Just nits, not a proper review.
> 2 spaces after date and 2 spaces before <, rather than just 1.
>
>>
>> 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.
>
> All lines in the ChangeLog should be tab indented, rather than just some of
> them and others with 8 spaces.

There's alsot contrib/git-commit-mklog.py, which provides a template for 
these (I also have trouble remembering the formatting rules).

>
>> --- 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 ""
>
> define_insn_and_split patterns better should have some name, even if it
> starts with *.  It makes dumps more readable, and you can refer to it
> in the ChangeLog when it is added or changed etc.
>
>> +  [(set (match_operand:GPR 0 "register_operand" "=r")
>> +    (match_operand:GPR 1 "splittable_const_int_operand" "i"))]
>> +  "cse_not_expected"
>> +  "#"
>> +  "&& 1"
>> +  [(const_int 0)]
>> +
>
> Why the empty line?
>
>> +{
>> +  riscv_move_integer (operands[0], operands[0], INTVAL (operands[1]),
>> +		      <GPR:MODE>mode, TRUE);
>
> You can just use <MODE> if there is only one iterator in the pattern.
>
> 	Jakub