Re: [x86 PATCH] PR rtl-optimization/107991: peephole2 to tweak register allocation.

[Richard Sandiford <richard.sandiford@arm.com>]

Uros Bizjak <ubizjak@gmail.com> writes:
> On Mon, Jan 9, 2023 at 4:01 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>>
>>
>> This patch addresses PR rtl-optimization/107991, which is a P2 regression
>> where GCC currently requires more "mov" instructions than GCC 7.
>>
>> The x86's two address ISA creates some interesting challenges for reload.
>> For example, the tricky "x = y - x" usually needs to be implemented on x86
>> as
>>
>>         tmp = x
>>         x = y
>>         x -= tmp
>>
>> where a scratch register and two mov's are required to work around
>> the lack of a subf (subtract from) or rsub (reverse subtract) insn.
>>
>> Not uncommonly, if y is dead after this subtraction, register allocation
>> can be improved by clobbering y.
>>
>>         y -= x
>>         x = y
>>
>> For the testcase in PR 107991, things are slightly more complicated,
>> where y is not itself dead, but is assigned from (i.e. equivalent to)
>> a value that is dead.  Hence we have something like:
>>
>>         y = z
>>         x = y - x
>>
>> so, GCC's reload currently generates the expected shuffle (as y is live):
>>
>>         y = z
>>         tmp = x
>>         x = y
>>         x -= tmp
>>
>> but we can use a peephole2 that understands that y and z are equivalent,
>> and that z is dead, to produce the shorter sequence:
>>
>>         y = z
>>         z -= x
>>         x = z
>>
>> In practice, for the new testcase from PR 107991, which before produced:
>>
>> foo:    movl    %edx, %ecx
>>         movl    %esi, %edx
>>         movl    %esi, %eax
>>         subl    %ecx, %edx
>>         testb   %dil, %dil
>>         cmovne  %edx, %eax
>>         ret
>>
>> with this patch/peephole2 we now produce the much improved:
>>
>> foo:    movl    %esi, %eax
>>         subl    %edx, %esi
>>         testb   %dil, %dil
>>         cmovne  %esi, %eax
>>         ret
>>
>>
>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
>> and make -k check, both with and without --target_board=unix{-m32},
>> with no new failures.  Ok for mainline?
>
> Looking at the PR, it looks to me that Richard S (CC'd) wants to solve
> this issue in the register allocator. This would be preferred
> (compared to a very specialized peephole2), since peephole2 pass comes
> very late in the game, so one freed register does not contribute to
> lower the register pressure at all.

Yeah, I think there are three issues that would all be good to fix:

(1) the fwprop regression
(2) the unhelpful pre-RA move
(3) the RA handling of what it sees now

I had a look last year to see where (1) was coming from, and it turned
out to be bad bookkeeping during a recursive walk.  I've got a couple of
competing ideas for how to fix it, but I've not had time to work on it
since then, sorry.

> Peephole2 should be used to clean after reload only in rare cases when
> target ISA prevents generic solution. From your description, a generic
> solution would benefit all targets with destructive subtraction (or
> perhaps also for other noncommutative operations).

Yeah, agree that peepholes aren't the best fit here.  The problem could
occur with instructions that are too far apart to be peepholed.

Thanks,
Richard

> So, please coordinate with Richard S regarding this issue.
>
> Thanks,
> Uros.
>
>>
>>
>> 2023-01-09  Roger Sayle  <roger@nextmovesoftware.com>
>>
>> gcc/ChangeLog
>>         PR rtl-optimization/107991
>>         * config/i386/i386.md (peephole2): New peephole2 to avoid register
>>         shuffling before a subtraction, after a register-to-register move.
>>
>> gcc/testsuite/ChangeLog
>>         PR rtl-optimization/107991
>>         * gcc.target/i386/pr107991.c: New test case.
>>
>>
>> Thanks in advance,
>> Roger
>> --
>>