* [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
@ 2016-05-17 10:02 Jiong Wang
2016-05-18 14:26 ` Vladimir Makarov
0 siblings, 1 reply; 6+ messages in thread
From: Jiong Wang @ 2016-05-17 10:02 UTC (permalink / raw)
To: GCC Patches; +Cc: Vladimir N Makarov
[-- Attachment #1: Type: text/plain, Size: 605 bytes --]
This bug is introduced by my commit r236181 where the inner rtx of
SUBREG haven't been checked while it should as "in_class_p" only
works with REG, and SUBREG_REG is actually not always REG. If REG_P
check failed, then we should fall back to normal code patch. The
following simple testcase for x86 can reproduce this bug.
long
foo (long a)
{
return (unsigned) foo;
}
OK for trunk?
x86-64 bootstrap OK and no regression on check-gcc/g++.
2016-05-17 Jiong Wang <jiong.wang@arm.com>
gcc/
PR rtl-optimization/71150
* lra-constraint (process_addr_reg): Guard "in_class_p" with REG_P check.
[-- Attachment #2: lra.patch --]
[-- Type: text/x-patch, Size: 632 bytes --]
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 56ab5b4..e4e6c8c 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p, rtx_insn **before, rtx_insn **aft
register, and this normally will be a subreg which should be reloaded
as a whole. This is particularly likely to be triggered when
-fno-split-wide-types specified. */
- if (in_class_p (reg, cl, &new_class)
+ if (!REG_P (reg)
+ || in_class_p (reg, cl, &new_class)
|| GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
loc = &SUBREG_REG (*loc);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
2016-05-17 10:02 [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P Jiong Wang
@ 2016-05-18 14:26 ` Vladimir Makarov
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Makarov @ 2016-05-18 14:26 UTC (permalink / raw)
To: Jiong Wang, GCC Patches
On 05/17/2016 06:02 AM, Jiong Wang wrote:
> This bug is introduced by my commit r236181 where the inner rtx of
> SUBREG haven't been checked while it should as "in_class_p" only
> works with REG, and SUBREG_REG is actually not always REG. If REG_P
> check failed, then we should fall back to normal code patch. The
> following simple testcase for x86 can reproduce this bug.
>
> long
> foo (long a)
> {
> return (unsigned) foo;
> }
>
> OK for trunk?
>
Yes. Thank you, Jiong.
> x86-64 bootstrap OK and no regression on check-gcc/g++.
>
> 2016-05-17 Jiong Wang <jiong.wang@arm.com>
>
> gcc/
> PR rtl-optimization/71150
> * lra-constraint (process_addr_reg): Guard "in_class_p" with REG_P
> check.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
2016-05-17 11:18 ` Jiong Wang
@ 2016-05-17 11:22 ` Uros Bizjak
0 siblings, 0 replies; 6+ messages in thread
From: Uros Bizjak @ 2016-05-17 11:22 UTC (permalink / raw)
To: Jiong Wang; +Cc: gcc-patches, Vladimir Makarov
On Tue, May 17, 2016 at 1:18 PM, Jiong Wang <jiong.wang@foss.arm.com> wrote:
> On 17/05/16 11:23, Uros Bizjak wrote:
>>
>> On Tue, May 17, 2016 at 12:17 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>
>>> Hello!
>>>
>>>> This bug is introduced by my commit r236181 where the inner rtx of
>>>> SUBREG haven't been checked while it should as "in_class_p" only
>>>> works with REG, and SUBREG_REG is actually not always REG. If REG_P
>>>> check failed, then we should fall back to normal code patch. The
>>>> following simple testcase for x86 can reproduce this bug.
>>>> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
>>>> index 56ab5b4..e4e6c8c 100644
>>>> --- a/gcc/lra-constraints.c
>>>> +++ b/gcc/lra-constraints.c
>>>> @@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p,
>>>> rtx_insn **before, rtx_insn **aft
>>>> register, and this normally will be a subreg which should be reloaded
>>>> as a whole. This is particularly likely to be triggered when
>>>> -fno-split-wide-types specified. */
>>>> - if (in_class_p (reg, cl, &new_class)
>>>> + if (!REG_P (reg)
>>>> + || in_class_p (reg, cl, &new_class)
>>>> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
>>>> loc = &SUBREG_REG (*loc);
>>>
>>> Why not check SUBREG_P instead of !REG_P?
>>
>> Or, alternatively:
>>
>> if ((REG_P && !in_class_p (reg, ...))
>> || GET_MODE_SIZE ...)
>>
>> Which is IMO much more readable.
>
>
> Thanks for review.
>
> I think your proposed rewrite will be the following,
>
> if (!(REG_P (reg) && !in_class_p (reg, cl, &new_class))
> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
>
> I feel the original code is easier to understand.
> The check logic is composed of three conditions, in a pre-requisite order,
> if one condition is true then reload the inner rtx, otherwise reload the
> whole subreg.
>
> if (!REG_P (reg)
> || in_class_p (reg, cl, &new_class)
> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
> loc = &SUBREG_REG (*loc);
Well, I don't want to bikeshed about it, it is OK with me either way.
(You still need a review of functionality from Vlad, though ...)
Thanks,
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
2016-05-17 10:23 ` Uros Bizjak
@ 2016-05-17 11:18 ` Jiong Wang
2016-05-17 11:22 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: Jiong Wang @ 2016-05-17 11:18 UTC (permalink / raw)
To: Uros Bizjak, gcc-patches; +Cc: Vladimir Makarov
On 17/05/16 11:23, Uros Bizjak wrote:
> On Tue, May 17, 2016 at 12:17 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Hello!
>>
>>> This bug is introduced by my commit r236181 where the inner rtx of
>>> SUBREG haven't been checked while it should as "in_class_p" only
>>> works with REG, and SUBREG_REG is actually not always REG. If REG_P
>>> check failed, then we should fall back to normal code patch. The
>>> following simple testcase for x86 can reproduce this bug.
>>> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
>>> index 56ab5b4..e4e6c8c 100644
>>> --- a/gcc/lra-constraints.c
>>> +++ b/gcc/lra-constraints.c
>>> @@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p, rtx_insn **before, rtx_insn **aft
>>> register, and this normally will be a subreg which should be reloaded
>>> as a whole. This is particularly likely to be triggered when
>>> -fno-split-wide-types specified. */
>>> - if (in_class_p (reg, cl, &new_class)
>>> + if (!REG_P (reg)
>>> + || in_class_p (reg, cl, &new_class)
>>> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
>>> loc = &SUBREG_REG (*loc);
>> Why not check SUBREG_P instead of !REG_P?
> Or, alternatively:
>
> if ((REG_P && !in_class_p (reg, ...))
> || GET_MODE_SIZE ...)
>
> Which is IMO much more readable.
Thanks for review.
I think your proposed rewrite will be the following,
if (!(REG_P (reg) && !in_class_p (reg, cl, &new_class))
|| GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
I feel the original code is easier to understand.
The check logic is composed of three conditions, in a pre-requisite order,
if one condition is true then reload the inner rtx, otherwise reload the
whole subreg.
if (!REG_P (reg)
|| in_class_p (reg, cl, &new_class)
|| GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
loc = &SUBREG_REG (*loc);
>
> Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
2016-05-17 10:17 Uros Bizjak
@ 2016-05-17 10:23 ` Uros Bizjak
2016-05-17 11:18 ` Jiong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2016-05-17 10:23 UTC (permalink / raw)
To: gcc-patches; +Cc: Jiong Wang, Vladimir Makarov
On Tue, May 17, 2016 at 12:17 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
>> This bug is introduced by my commit r236181 where the inner rtx of
>> SUBREG haven't been checked while it should as "in_class_p" only
>> works with REG, and SUBREG_REG is actually not always REG. If REG_P
>> check failed, then we should fall back to normal code patch. The
>> following simple testcase for x86 can reproduce this bug.
>
>> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
>> index 56ab5b4..e4e6c8c 100644
>> --- a/gcc/lra-constraints.c
>> +++ b/gcc/lra-constraints.c
>> @@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p, rtx_insn **before, rtx_insn **aft
>> register, and this normally will be a subreg which should be reloaded
>> as a whole. This is particularly likely to be triggered when
>> -fno-split-wide-types specified. */
>>- if (in_class_p (reg, cl, &new_class)
>>+ if (!REG_P (reg)
>>+ || in_class_p (reg, cl, &new_class)
>> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
>> loc = &SUBREG_REG (*loc);
>
> Why not check SUBREG_P instead of !REG_P?
Or, alternatively:
if ((REG_P && !in_class_p (reg, ...))
|| GET_MODE_SIZE ...)
Which is IMO much more readable.
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P
@ 2016-05-17 10:17 Uros Bizjak
2016-05-17 10:23 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2016-05-17 10:17 UTC (permalink / raw)
To: gcc-patches; +Cc: Jiong Wang, Vladimir Makarov
Hello!
> This bug is introduced by my commit r236181 where the inner rtx of
> SUBREG haven't been checked while it should as "in_class_p" only
> works with REG, and SUBREG_REG is actually not always REG. If REG_P
> check failed, then we should fall back to normal code patch. The
> following simple testcase for x86 can reproduce this bug.
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index 56ab5b4..e4e6c8c 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -1317,7 +1317,8 @@ process_addr_reg (rtx *loc, bool check_only_p, rtx_insn **before, rtx_insn **aft
> register, and this normally will be a subreg which should be reloaded
> as a whole. This is particularly likely to be triggered when
> -fno-split-wide-types specified. */
>- if (in_class_p (reg, cl, &new_class)
>+ if (!REG_P (reg)
>+ || in_class_p (reg, cl, &new_class)
> || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (ptr_mode))
> loc = &SUBREG_REG (*loc);
Why not check SUBREG_P instead of !REG_P?
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-18 14:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-17 10:02 [Patch] PR rtl-optimization/71150, guard in_class_p check with REG_P Jiong Wang
2016-05-18 14:26 ` Vladimir Makarov
2016-05-17 10:17 Uros Bizjak
2016-05-17 10:23 ` Uros Bizjak
2016-05-17 11:18 ` Jiong Wang
2016-05-17 11:22 ` Uros Bizjak
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).