* [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
@ 2019-01-07 14:42 Andre Vieira (lists)
2019-01-07 22:50 ` Jeff Law
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2019-01-07 14:42 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]
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.
[-- Attachment #2: use_hard_regs_p.patch --]
[-- Type: text/x-patch, Size: 950 bytes --]
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index c061093ed699620afe2dfda60d58066d6967523a..736b084acc552b75ff4d369b6584bc9ab422e21b 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1761,11 +1761,21 @@ uses_hard_regs_p (rtx x, HARD_REG_SET set)
return false;
code = GET_CODE (x);
mode = GET_MODE (x);
+
if (code == SUBREG)
{
+ /* For all SUBREGs we want to check whether the full multi-register
+ overlaps the set. For normal SUBREGs this means 'get_hard_regno' of
+ the inner register, for paradoxical SUBREGs this means the
+ 'get_hard_regno' of the full SUBREG and for complete SUBREGs either is
+ fine. Use the wider mode for all cases. */
+ rtx subreg = SUBREG_REG (x);
mode = wider_subreg_mode (x);
- x = SUBREG_REG (x);
- code = GET_CODE (x);
+ if (mode == GET_MODE (subreg))
+ {
+ x = subreg;
+ code = GET_CODE (x);
+ }
}
if (REG_P (x))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-01-07 14:42 [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs Andre Vieira (lists)
@ 2019-01-07 22:50 ` Jeff Law
2019-01-08 15:21 ` Andre Vieira (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Law @ 2019-01-07 22:50 UTC (permalink / raw)
To: Andre Vieira (lists), gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-01-07 22:50 ` Jeff Law
@ 2019-01-08 15:21 ` Andre Vieira (lists)
2019-01-11 22:54 ` Jeff Law
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2019-01-08 15:21 UTC (permalink / raw)
To: Jeff Law, gcc-patches; +Cc: vmakarov, richard.sandiford
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.
Suggestions welcome!!
Cheers,
Andre
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-01-08 15:21 ` Andre Vieira (lists)
@ 2019-01-11 22:54 ` Jeff Law
2019-02-01 14:31 ` Andre Vieira (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Law @ 2019-01-11 22:54 UTC (permalink / raw)
To: Andre Vieira (lists), gcc-patches; +Cc: vmakarov, richard.sandiford
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-01-11 22:54 ` Jeff Law
@ 2019-02-01 14:31 ` Andre Vieira (lists)
2019-02-13 10:55 ` Andre Vieira (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2019-02-01 14:31 UTC (permalink / raw)
To: Jeff Law, gcc-patches; +Cc: vmakarov, richard.sandiford
[-- Attachment #1: Type: text/plain, Size: 5224 bytes --]
On 11/01/2019 22:54, Jeff Law wrote:
> 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
>
Hi Jeff,
Thank you, I had a look but I don't think it is the same issue and even
though I can no longer reproduce this particular issue on trunk I do
believe the latent fault is still there and I would like to fix it.
As I mentioned in the earlier emails I forgot to make sure 'SUBREG_P
(x)' was also handled where we previously only accepted 'REG_P (x)' as
we would always strip away the parent subreg. I have also added the
testcase that triggered this issue on arm in previous GCC versions. This
does not trigger it on trunk any more due to other codegen changes.
Bootstrapped on aarch64, arm and x86, no regressions.
Is this OK for trunk?
gcc/ChangeLog:
2019-02-01 Andre Vieira <andre.simoesdiasvieira@arm.com>
PR target/86487
* lra-constraints.c(uses_hard_regs_p): Fix handling of
paradoxical SUBREGS.
gcc/testsuiteChangeLog:
2019-02-01 Andre Vieira <andre.simoesdiasvieira@arm.com>
PR target/86487
* gcc.target/arm/pr86487.c: New.
[-- Attachment #2: use_hard_regs_p.patch --]
[-- Type: text/x-patch, Size: 950 bytes --]
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index c061093ed699620afe2dfda60d58066d6967523a..736b084acc552b75ff4d369b6584bc9ab422e21b 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1761,11 +1761,21 @@ uses_hard_regs_p (rtx x, HARD_REG_SET set)
return false;
code = GET_CODE (x);
mode = GET_MODE (x);
+
if (code == SUBREG)
{
+ /* For all SUBREGs we want to check whether the full multi-register
+ overlaps the set. For normal SUBREGs this means 'get_hard_regno' of
+ the inner register, for paradoxical SUBREGs this means the
+ 'get_hard_regno' of the full SUBREG and for complete SUBREGs either is
+ fine. Use the wider mode for all cases. */
+ rtx subreg = SUBREG_REG (x);
mode = wider_subreg_mode (x);
- x = SUBREG_REG (x);
- code = GET_CODE (x);
+ if (mode == GET_MODE (subreg))
+ {
+ x = subreg;
+ code = GET_CODE (x);
+ }
}
if (REG_P (x))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-02-01 14:31 ` Andre Vieira (lists)
@ 2019-02-13 10:55 ` Andre Vieira (lists)
2019-02-13 16:46 ` Vladimir Makarov
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2019-02-13 10:55 UTC (permalink / raw)
To: gcc-patches; +Cc: vmakarov, richard.sandiford
PING.
Since Jeff is away can another maintainer have a look at this please?
Cheers,
Andre
On 01/02/2019 14:31, Andre Vieira (lists) wrote:
>
>
> On 11/01/2019 22:54, Jeff Law wrote:
>> 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
>>
>
> Hi Jeff,
>
> Thank you, I had a look but I don't think it is the same issue and even
> though I can no longer reproduce this particular issue on trunk I do
> believe the latent fault is still there and I would like to fix it.
>
>
> As I mentioned in the earlier emails I forgot to make sure 'SUBREG_P
> (x)' was also handled where we previously only accepted 'REG_P (x)' as
> we would always strip away the parent subreg. I have also added the
> testcase that triggered this issue on arm in previous GCC versions. This
> does not trigger it on trunk any more due to other codegen changes.
>
>
> Bootstrapped on aarch64, arm and x86, no regressions.
>
> Is this OK for trunk?
>
>
> gcc/ChangeLog:
> 2019-02-01 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
>
> Â Â Â Â Â Â Â PR target/86487
> Â Â Â Â Â Â Â * lra-constraints.c(uses_hard_regs_p): Fix handling of
> paradoxical SUBREGS.
>
> gcc/testsuiteChangeLog:
> 2019-02-01 Andre Vieira <andre.simoesdiasvieira@arm.com>
>
>
> Â Â Â Â Â Â Â PR target/86487
> Â Â Â Â Â Â Â * gcc.target/arm/pr86487.c: New.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-02-13 10:55 ` Andre Vieira (lists)
@ 2019-02-13 16:46 ` Vladimir Makarov
2019-02-15 11:35 ` Andre Vieira (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Makarov @ 2019-02-13 16:46 UTC (permalink / raw)
To: Andre Vieira (lists), gcc-patches; +Cc: richard.sandiford
On 2019-02-13 5:54 a.m., Andre Vieira (lists) wrote:
> PING.
>
> Since Jeff is away can another maintainer have a look at this please?
>
>
I see the following patch
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index c061093ed699620afe2dfda60d58066d6967523a..736b084acc552b75ff4d369b6584bc9ab422e21b 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1761,11 +1761,21 @@ uses_hard_regs_p (rtx x, HARD_REG_SET set)
return false;
code = GET_CODE (x);
mode = GET_MODE (x);
+
if (code == SUBREG)
{
+ /* For all SUBREGs we want to check whether the full multi-register
+ overlaps the set. For normal SUBREGs this means 'get_hard_regno' of
+ the inner register, for paradoxical SUBREGs this means the
+ 'get_hard_regno' of the full SUBREG and for complete SUBREGs either is
+ fine. Use the wider mode for all cases. */
+ rtx subreg = SUBREG_REG (x);
mode = wider_subreg_mode (x);
- x = SUBREG_REG (x);
- code = GET_CODE (x);
+ if (mode == GET_MODE (subreg))
+ {
+ x = subreg;
+ code = GET_CODE (x);
+ }
}
if (REG_P (x))
In your case, x will be SUBREG and be processed recursively only as a register of subreg.
I think you need to change the last line on
if (REG_P (x) || code == SUBREG)
then the subreg will processed by get_hard_regno as subreg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-02-13 16:46 ` Vladimir Makarov
@ 2019-02-15 11:35 ` Andre Vieira (lists)
2019-02-19 4:27 ` Vladimir Makarov
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2019-02-15 11:35 UTC (permalink / raw)
To: Vladimir Makarov, gcc-patches; +Cc: richard.sandiford
[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]
Hi Vlad,
On 13/02/2019 16:46, Vladimir Makarov wrote:
>
> On 2019-02-13 5:54 a.m., Andre Vieira (lists) wrote:
>> PING.
>>
>> Since Jeff is away can another maintainer have a look at this please?
>>
>>
> I see the following patch
Yeah I uploaded the wrong patch... sorry. See attached, including a
testcase, currently only fails on GCC-8 and previous though.
>
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index
> c061093ed699620afe2dfda60d58066d6967523a..736b084acc552b75ff4d369b6584bc9ab422e21b
> 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -1761,11 +1761,21 @@ uses_hard_regs_p (rtx x, HARD_REG_SET set)
> Â Â Â Â return false;
> Â Â code = GET_CODE (x);
> Â Â mode = GET_MODE (x);
> +
> Â Â if (code == SUBREG)
> Â Â Â Â {
> +Â Â Â Â Â /* For all SUBREGs we want to check whether the full multi-register
> +    overlaps the set. For normal SUBREGs this means 'get_hard_regno' of
> +Â Â Â Â the inner register, for paradoxical SUBREGs this means the
> +Â Â Â Â 'get_hard_regno' of the full SUBREG and for complete SUBREGs
> either is
> +    fine. Use the wider mode for all cases. */
> +Â Â Â Â Â rtx subreg = SUBREG_REG (x);
> Â Â Â Â Â Â mode = wider_subreg_mode (x);
> -Â Â Â Â Â x = SUBREG_REG (x);
> -Â Â Â Â Â code = GET_CODE (x);
> +Â Â Â Â Â if (mode == GET_MODE (subreg))
> +Â Â Â {
> +Â Â Â Â Â x = subreg;
> +Â Â Â Â Â code = GET_CODE (x);
> +Â Â Â }
> Â Â Â Â }
>
> Â Â if (REG_P (x))
>
> In your case, x will be SUBREG and be processed recursively only as a
> register of subreg.
>
> I think you need to change the last line on
>
> Â if (REG_P (x) || code == SUBREG)
>
> then the subreg will processed by get_hard_regno as subreg.
>
[-- Attachment #2: use_hard_regs_p.patch --]
[-- Type: text/x-patch, Size: 1724 bytes --]
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 0ef13439b5055dd2c5d5049d7f62f6b3b1ddfe2a..3af41b6eed2dc113c3158e1bde1c65a896d8feb5 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1761,14 +1761,24 @@ uses_hard_regs_p (rtx x, HARD_REG_SET set)
return false;
code = GET_CODE (x);
mode = GET_MODE (x);
+
if (code == SUBREG)
{
+ /* For all SUBREGs we want to check whether the full multi-register
+ overlaps the set. For normal SUBREGs this means 'get_hard_regno' of
+ the inner register, for paradoxical SUBREGs this means the
+ 'get_hard_regno' of the full SUBREG and for complete SUBREGs either is
+ fine. Use the wider mode for all cases. */
+ rtx subreg = SUBREG_REG (x);
mode = wider_subreg_mode (x);
- x = SUBREG_REG (x);
- code = GET_CODE (x);
+ if (mode == GET_MODE (subreg))
+ {
+ x = subreg;
+ code = GET_CODE (x);
+ }
}
- if (REG_P (x))
+ if (REG_P (x) || SUBREG_P (x))
{
x_hard_regno = get_hard_regno (x, true);
return (x_hard_regno >= 0
diff --git a/gcc/testsuite/gcc.target/arm/pr86487.c b/gcc/testsuite/gcc.target/arm/pr86487.c
new file mode 100644
index 0000000000000000000000000000000000000000..1c1db7852d91a82a1d2b6eaa4f3d4c6dbef107f5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr86487.c
@@ -0,0 +1,10 @@
+/* { dg-skip-if "" { *-*-* } { "-march=armv[0-6]*" "-mthumb" } { "" } } */
+/* { dg-require-effective-target arm_neon_hw } */
+/* { dg-options "-O1 -mbig-endian" } */
+/* { dg-add-options arm_neon } */
+int a, b, c, d;
+long long fn1(long long p2) { return p2 == 0 ? -1 : -1 % p2; }
+void fn2(long long p1, short p2, long p3) {
+ b = fn1((d || 6) & a);
+ c = b | p3;
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs
2019-02-15 11:35 ` Andre Vieira (lists)
@ 2019-02-19 4:27 ` Vladimir Makarov
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Makarov @ 2019-02-19 4:27 UTC (permalink / raw)
To: Andre Vieira (lists), gcc-patches; +Cc: richard.sandiford
On 2019-02-15 6:35 a.m., Andre Vieira (lists) wrote:
> Hi Vlad,
>
> On 13/02/2019 16:46, Vladimir Makarov wrote:
>>
>> On 2019-02-13 5:54 a.m., Andre Vieira (lists) wrote:
>>> PING.
>>>
>>> Since Jeff is away can another maintainer have a look at this please?
>>>
>>>
>> I see the following patch
>
> Yeah I uploaded the wrong patch... sorry. See attached, including a
> testcase, currently only fails on GCC-8 and previous though.
>>
It happens. The new version is ok to commit. Thank you for working on
the patch.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-19 4:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-07 14:42 [PATCH, GCC] PR target/86487: fix the way 'uses_hard_regs_p' handles paradoxical subregs Andre Vieira (lists)
2019-01-07 22:50 ` Jeff Law
2019-01-08 15:21 ` Andre Vieira (lists)
2019-01-11 22:54 ` Jeff Law
2019-02-01 14:31 ` Andre Vieira (lists)
2019-02-13 10:55 ` Andre Vieira (lists)
2019-02-13 16:46 ` Vladimir Makarov
2019-02-15 11:35 ` Andre Vieira (lists)
2019-02-19 4:27 ` Vladimir Makarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).