* [patch 79279] combine/simplify_set issue
@ 2017-01-30 9:47 Aurelien Buhrig
2017-01-31 21:15 ` Segher Boessenkool
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Buhrig @ 2017-01-30 9:47 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
Hi,
This patch fixes a combiner bug in simplify_set which calls
CANNOT_CHANGE_MODE_CLASS with wrong mode params.
It occurs when trying to simplify paradoxical subregs of hw regs (whose
natural mode is lower than a word).
In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2
x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false
and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG
(src))
See:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79279
Validated on a private target,
bootstraped on x86_64-pc-linux-gnu, but I'm not sure which target is the
most relevant for this patch though ...
OK to commit?
Aurélien
[-- Attachment #2: patch_trunk.diff --]
[-- Type: text/plain, Size: 659 bytes --]
Changelog:
2017-01-27 Aurelien Buhrig <aurelien.buhrig.gcc@gmail.com>
* combine.c (simplify_set): Fix call to REG_CANNOT_CHANGE_MODE_CLASS_P
Index: gcc/combine.c
===================================================================
--- gcc/combine.c (revision 244978)
+++ gcc/combine.c (working copy)
@@ -6796,8 +6796,8 @@
#ifdef CANNOT_CHANGE_MODE_CLASS
&& ! (REG_P (dest) && REGNO (dest) < FIRST_PSEUDO_REGISTER
&& REG_CANNOT_CHANGE_MODE_P (REGNO (dest),
- GET_MODE (SUBREG_REG (src)),
- GET_MODE (src)))
+ GET_MODE (src),
+ GET_MODE (SUBREG_REG (src))))
#endif
&& (REG_P (dest)
|| (GET_CODE (dest) == SUBREG
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 79279] combine/simplify_set issue
2017-01-30 9:47 [patch 79279] combine/simplify_set issue Aurelien Buhrig
@ 2017-01-31 21:15 ` Segher Boessenkool
2017-02-01 9:04 ` Aurelien Buhrig
0 siblings, 1 reply; 6+ messages in thread
From: Segher Boessenkool @ 2017-01-31 21:15 UTC (permalink / raw)
To: Aurelien Buhrig; +Cc: gcc-patches
Hello,
On Mon, Jan 30, 2017 at 10:43:23AM +0100, Aurelien Buhrig wrote:
> This patch fixes a combiner bug in simplify_set which calls
> CANNOT_CHANGE_MODE_CLASS with wrong mode params.
> It occurs when trying to simplify paradoxical subregs of hw regs (whose
> natural mode is lower than a word).
>
> In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2
> x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false
> and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG
> (src))
r62212 (in 2003) changed it to what we have now, it used to be what you
want to change it back to.
You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined.
Where does this transformation go wrong? Why is the resulting insn
recognised at all? For example, general_operand should refuse it.
Maybe you have custom *_operand that do not handle subreg correctly?
The existing code looks correct: what we want to know is if an m2
interpreted as an m1 yields the correct value. We might *also* want
your condition, but I'm not sure about that.
> See:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79279
>
> Validated on a private target,
> bootstraped on x86_64-pc-linux-gnu, but I'm not sure which target is the
> most relevant for this patch though ...
>
> OK to commit?
Sorry, no. We're currently in development stage 4, and this is not a
regression (see <https://gcc.gnu.org/develop.html#stage4>). But we can
of course discuss this further, and you can repost the patch when stage 1
opens (a few months from now) if you still want it.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 79279] combine/simplify_set issue
2017-01-31 21:15 ` Segher Boessenkool
@ 2017-02-01 9:04 ` Aurelien Buhrig
2017-02-01 19:12 ` Segher Boessenkool
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Buhrig @ 2017-02-01 9:04 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
On 31/01/2017 22:15, Segher Boessenkool wrote:
> Hello,
>
> On Mon, Jan 30, 2017 at 10:43:23AM +0100, Aurelien Buhrig wrote:
>> This patch fixes a combiner bug in simplify_set which calls
>> CANNOT_CHANGE_MODE_CLASS with wrong mode params.
>> It occurs when trying to simplify paradoxical subregs of hw regs (whose
>> natural mode is lower than a word).
>>
>> In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2
>> x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false
>> and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG
>> (src))
> r62212 (in 2003) changed it to what we have now, it used to be what you
> want to change it back to.
>
> You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined.
No, just some hard regs whose natural mode size is 2 and UNIT_PER_WORD
is 4...
>
> Where does this transformation go wrong? Why is the resulting insn
> recognised at all? For example, general_operand should refuse it.
> Maybe you have custom *_operand that do not handle subreg correctly?
>
> The existing code looks correct: what we want to know is if an m2
> interpreted as an m1 yields the correct value. We might *also* want
> your condition, but I'm not sure about that.
OK, looks like both m1->m2 & m2 -> m1 checks would be needed, but the m1
-> m2 should be filtererd by valid predicates (general_operand).
Sorry about that.
>> See:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79279
>>
>> Validated on a private target,
>> bootstraped on x86_64-pc-linux-gnu, but I'm not sure which target is the
>> most relevant for this patch though ...
>>
>> OK to commit?
> Sorry, no. We're currently in development stage 4, and this is not a
> regression (see <https://gcc.gnu.org/develop.html#stage4>). But we can
> of course discuss this further, and you can repost the patch when stage 1
> opens (a few months from now) if you still want it.
OK, but not sure if it needs to be patched any more.
Aurélien
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 79279] combine/simplify_set issue
2017-02-01 9:04 ` Aurelien Buhrig
@ 2017-02-01 19:12 ` Segher Boessenkool
2017-02-02 10:27 ` Aurelien Buhrig
0 siblings, 1 reply; 6+ messages in thread
From: Segher Boessenkool @ 2017-02-01 19:12 UTC (permalink / raw)
To: Aurelien Buhrig; +Cc: gcc-patches
Hi Aurelien,
On Wed, Feb 01, 2017 at 10:02:56AM +0100, Aurelien Buhrig wrote:
> >> This patch fixes a combiner bug in simplify_set which calls
> >> CANNOT_CHANGE_MODE_CLASS with wrong mode params.
> >> It occurs when trying to simplify paradoxical subregs of hw regs (whose
> >> natural mode is lower than a word).
> >>
> >> In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2
> >> x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false
> >> and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG
> >> (src))
> > r62212 (in 2003) changed it to what we have now, it used to be what you
> > want to change it back to.
> >
> > You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined.
> No, just some hard regs whose natural mode size is 2 and UNIT_PER_WORD
> is 4...
You said it is a paradoxical subreg? Or do you mean the result is
a paradoxical subreg?
> > Where does this transformation go wrong? Why is the resulting insn
> > recognised at all? For example, general_operand should refuse it.
> > Maybe you have custom *_operand that do not handle subreg correctly?
> >
> > The existing code looks correct: what we want to know is if an m2
> > interpreted as an m1 yields the correct value. We might *also* want
> > your condition, but I'm not sure about that.
> OK, looks like both m1->m2 & m2 -> m1 checks would be needed, but the m1
> -> m2 should be filtererd by valid predicates (general_operand).
> Sorry about that.
Hrm, maybe you can show the RTL before and after this transform?
> >> OK to commit?
> > Sorry, no. We're currently in development stage 4, and this is not a
> > regression (see <https://gcc.gnu.org/develop.html#stage4>). But we can
> > of course discuss this further, and you can repost the patch when stage 1
> > opens (a few months from now) if you still want it.
> OK, but not sure if it needs to be patched any more.
Let's work that out then.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 79279] combine/simplify_set issue
2017-02-01 19:12 ` Segher Boessenkool
@ 2017-02-02 10:27 ` Aurelien Buhrig
2017-02-03 0:37 ` Segher Boessenkool
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Buhrig @ 2017-02-02 10:27 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
Hi Segher,
>>>> This patch fixes a combiner bug in simplify_set which calls
>>>> CANNOT_CHANGE_MODE_CLASS with wrong mode params.
>>>> It occurs when trying to simplify paradoxical subregs of hw regs (whose
>>>> natural mode is lower than a word).
>>>>
>>>> In fact, changing from (set x:m1 (subreg:m1 (op:m2))) to (set (subreg:m2
>>>> x) op2) is valid if REG_CANNOT_CHANGE_MODE_P (x, m1, m2) is false
>>>> and REG_CANNOT_CHANGE_MODE_P (x, GET_MODE (src), GET_MODE (SUBREG_REG
>>>> (src))
>>> r62212 (in 2003) changed it to what we have now, it used to be what you
>>> want to change it back to.
>>>
>>> You say m1 > m2, which means you have WORD_REGISTER_OPERATIONS defined.
>> No, just some hard regs whose natural mode size is 2 and UNIT_PER_WORD
>> is 4...
> You said it is a paradoxical subreg? Or do you mean the result is
> a paradoxical subreg?
I mean that the result is a paradoxical subreg: (subreg:SI (reg:HI r0)
0) = (reg:SI r0) whith r0 is a HI reg.
>>> Where does this transformation go wrong? Why is the resulting insn
>>> recognised at all? For example, general_operand should refuse it.
>>> Maybe you have custom *_operand that do not handle subreg correctly?
>>>
>>> The existing code looks correct: what we want to know is if an m2
>>> interpreted as an m1 yields the correct value. We might *also* want
>>> your condition, but I'm not sure about that.
>> OK, looks like both m1->m2 & m2 -> m1 checks would be needed, but the m1
>> -> m2 should be filtererd by valid predicates (general_operand).
>> Sorry about that.
> Hrm, maybe you can show the RTL before and after this transform?
RTL before combine:
(set (reg:SI 31 (lshiftt:SI (reg:SI 29) (const_int 16))))
(set (reg:HI 1 "r1") (reg:HI 25))
(set (reg:HI 0 "r0") (subreg:HI (reg:SI 31) 0)) ; LE target
r0 and r1 are HI regs
RTL after combining 1 --> 3:
(set (reg:HI 1 "r1") (reg:HI 25))
(set (reg:SI 0 "r0") (lshift:SI (reg:SI 29) (const_int 16)))
and r1 is clobbered by last insn.
Thanks,
Aurélien
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 79279] combine/simplify_set issue
2017-02-02 10:27 ` Aurelien Buhrig
@ 2017-02-03 0:37 ` Segher Boessenkool
0 siblings, 0 replies; 6+ messages in thread
From: Segher Boessenkool @ 2017-02-03 0:37 UTC (permalink / raw)
To: Aurelien Buhrig; +Cc: gcc-patches
On Thu, Feb 02, 2017 at 11:27:12AM +0100, Aurelien Buhrig wrote:
> > Hrm, maybe you can show the RTL before and after this transform?
> RTL before combine:
> (set (reg:SI 31 (lshiftt:SI (reg:SI 29) (const_int 16))))
> (set (reg:HI 1 "r1") (reg:HI 25))
> (set (reg:HI 0 "r0") (subreg:HI (reg:SI 31) 0)) ; LE target
>
> r0 and r1 are HI regs
>
> RTL after combining 1 --> 3:
> (set (reg:HI 1 "r1") (reg:HI 25))
> (set (reg:SI 0 "r0") (lshift:SI (reg:SI 29) (const_int 16)))
> and r1 is clobbered by last insn.
Ah, much clearer now, thanks!
There is this test:
&& (((GET_MODE_SIZE (GET_MODE (src)) + (UNITS_PER_WORD - 1))
/ UNITS_PER_WORD)
== ((GET_MODE_SIZE (GET_MODE (SUBREG_REG (src)))
+ (UNITS_PER_WORD - 1)) / UNITS_PER_WORD))
On most targets the size of a register is (at least) UNITS_PER_WORD, so
this works there. But this code should really use can_change_dest_mode,
at least for hard registers, which does
if (regno < FIRST_PSEUDO_REGISTER)
return (HARD_REGNO_MODE_OK (regno, mode)
&& REG_NREGS (x) >= hard_regno_nregs[regno][mode]);
i.e. it tests the new mode does not use more hard registers than the
old one did, which indeed would be bad.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-03 0:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-30 9:47 [patch 79279] combine/simplify_set issue Aurelien Buhrig
2017-01-31 21:15 ` Segher Boessenkool
2017-02-01 9:04 ` Aurelien Buhrig
2017-02-01 19:12 ` Segher Boessenkool
2017-02-02 10:27 ` Aurelien Buhrig
2017-02-03 0:37 ` Segher Boessenkool
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).