[PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs

[Jeff Law]

On 1/8/19 8:21 AM, Andre Vieira (lists) wrote:
> 
> 
> On 07/01/2019 22:50, Jeff Law wrote:
>> On 1/7/19 7:42 AM, Andre Vieira (lists) wrote:
>>> Hi,
>>>
>>> This patch fixes the way 'uses_hard_regs_p' handles paradoxical subregs.
>>> Â Â The function is supposed to detect whether a register access of 'x'
>>> overlaps with 'set'.  For SUBREGs it should check whether any of the
>>> full multi-register overlaps with 'set'.  The former behavior used to
>>> grab the widest mode of the inner/outer registers of a SUBREG and the
>>> inner register, and check all registers from the inner-register onwards
>>> for the given width.  For normal SUBREGS this gives you the full
>>> register, for paradoxical SUBREGS however it may give you the wrong set
>>> of registers if the index is not the first of the multi-register set.
>>>
>>> The original error reported in PR target/86487 can no longer be
>>> reproduced with the given test, this was due to an unrelated code-gen
>>> change, regardless I believe this should still be fixed as it is simply
>>> wrong behavior by uses_hard_regs_p which may be triggered by a different
>>> test-case or by future changes to the compiler.  Also it is useful to
>>> point out that this isn't actually a 'target' issue as this code could
>>> potentially hit any other target using paradoxical SUBREGS.  Should I
>>> change the Bugzilla ticket to reflect this is actually a target agnostic
>>> issue in RTL?
>>>
>>> There is a gotcha here, I don't know what would happen if you hit the
>>> cases of get_hard_regno where it would return -1, quoting the comment
>>> above that function "If X is not a register or a subreg of a register,
>>> return -1." though I think if we are hitting this then things must have
>>> gone wrong before?
>>>
>>> Bootstrapped on aarch64, arm and x86, no regressions.
>>>
>>> Is this OK for trunk?
>>>
>>>
>>> gcc/ChangeLog:
>>> 2019-01-07 Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>>
>>>
>>> Â Â Â Â Â Â Â Â  PR target/86487
>>> Â Â Â Â Â Â Â Â  * lra-constraints.c(uses_hard_regs_p): Fix handling of
>>> paradoxical SUBREGS.
>> But doesn't wider_subreg_mode give us the wider of the two modes here
>> and we use that wider mode when we call overlaps_hard_reg_set_p which
>> should ultimately check all the registers in the paradoxical.
>>
>> I must be missing something here?!?
>>
>> jeff
>>
> 
> Hi Jeff,
> 
> It does give us the wider of the two modes, but we also then grab the
> "subreg" of the paradoxical subreg.  If you look at the first example
> case of the bugzilla ticket, for an older gcc (say gcc-8) and the
> options provided (using big-endian), it will generate the following subreg:
> (subreg:DI (reg:SI 122) 0)
> 
> This paradoxical subreg represents a register pair r0-r1, where because
> of big-endian and subgreg index 0, r1 is the value we care about and r0
> the one we say "it can be whatever" by using this paradoxical subreg.
> 
> When 'uses_hard_regs_p' sees this as a subreg, it sets 'mode' to the
> wider, i.e. DImode, but it also sets 'x' to the subreg i.e. 'reg:SI
> 122', for which get_hard_regno correctly returns 'r1'.  But if you now
> pass 'overlaps_hard_reg_set_p' DImode and 'r1', it will check whether
> 'set' contains either 'r1-r2', and not 'r1'r0'.
> 
> To reproduce this again I now applied this patch to GCC 8 and found an
> issue with it. 'REG_P (x)' returns false if x is a 'SUBREG'. So I will
> need to change the later check to also include 'SUBREG_P (x)', I guess I
> was testing with a too new version of gcc that didn't lead to the bogus
> register allocation...
> 
> Which really encourages me to add some sort of testcase, but I'd very
> much like to come up with a less flaky one, we basically need to force
> the generation of a paradoxical subreg 'x', where 'get_hard_regno
> (SUBREG_REG (x)) != get_hard_regno (x)'.  This will cause
> 'uses_hard_regs_p' to give you a wrong answer.
BTW, you might look at 87305 which is another problem with big endian
pseudos and paradoxical subregs that Vlad just fixed.

jeff