diff mbox

[79279] combine/simplify_set issue

Message ID e3afa5c5-8338-d0ae-19e6-33cbda243d53@gmail.com
State New
Headers show

Commit Message

Aurelien Buhrig Jan. 30, 2017, 9:43 a.m. UTC 
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
Changelog:
2017-01-27  Aurelien Buhrig  <aurelien.buhrig.gcc@gmail.com>
	* combine.c (simplify_set): Fix call to REG_CANNOT_CHANGE_MODE_CLASS_P

Comments

Segher Boessenkool Jan. 31, 2017, 9:15 p.m. UTC | #1 
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
Aurelien Buhrig Feb. 1, 2017, 9:02 a.m. UTC | #2 
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
Segher Boessenkool Feb. 1, 2017, 7:12 p.m. UTC | #3 
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
Aurelien Buhrig Feb. 2, 2017, 10:27 a.m. UTC | #4 
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
Segher Boessenkool Feb. 3, 2017, 12:37 a.m. UTC | #5 
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
diff mbox

Patch

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