Patchwork RFA: patch to fix PR55116

login
register
mail settings
Submitter Richard Sandiford
Date Oct. 30, 2012, 11:09 a.m.
Message ID <87390wcj21.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com>
Download mbox | patch
Permalink /patch/195444/
State New
Headers show

Comments

Richard Sandiford - Oct. 30, 2012, 11:09 a.m.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Mon, Oct 29, 2012 at 5:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Oct 29, 2012 at 4:41 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Oct 29, 2012 at 9:38 AM, Vladimir Makarov
>>> <vmakarov@redhat.com> wrote:
>>>> On 12-10-29 12:21 PM, Richard Sandiford wrote:
>>>>>
>>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>>>
>>>>>>     H.J. in
>>>>>>
>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55116
>>>>>>
>>>>>>     reported an interesting address
>>>>>>
>>>>>> (and:DI (subreg:DI (plus:SI (ashift:SI (reg:SI 96 [ glob_vol_int.22 ])
>>>>>>                   (const_int 2 [0x2]))
>>>>>>               (symbol_ref:SI ("glob_vol_int_arr") <var_decl
>>>>>> 0x7ffff03c2720 glob_vol_int_arr>)) 0)
>>>>>>       (const_int 4294967295 [0xffffffff]))
>>>>>>
>>>>>>     which can not be correctly extracted.  Here `and' with `subreg'
>>>>>> behaves as an address mutation.
>>>>>>
>>>>>>     The following patch fixes the problem.
>>>>>>
>>>>>> Ok to commit, Richard?
>>>>>
>>>>> Heh, I wondered if subregs might still be used like that, and was almost
>>>>> tempted to add them just in case.
>>>>>
>>>>> I think this particular case is really a failed canonicalisation and that:
>>>>>
>>>>>     (and:DI (subreg:DI (foo:SI ...) 0) (const_int 0xffffffff))
>>>>>
>>>>> ought to be:
>>>>>
>>>>>     (zero_extend:DI (foo:SI ...))
>>>>
>>>> Yes, that was my thought too.
>>>>
>>>>> But I know I've approved MIPS patches to accept
>>>>> (and:DI ... (const_int 0xffffffff)) as an alternative.
>>>>>
>>>>>> Index: rtlanal.c
>>>>>> ===================================================================
>>>>>> --- rtlanal.c   (revision 192942)
>>>>>> +++ rtlanal.c   (working copy)
>>>>>> @@ -5459,6 +5459,11 @@ strip_address_mutations (rtx *loc, enum
>>>>>>          else if (code == AND && CONST_INT_P (XEXP (*loc, 1)))
>>>>>>           /* (and ... (const_int -X)) is used to align to X bytes.  */
>>>>>>           loc = &XEXP (*loc, 0);
>>>>>> +      else if (code == SUBREG
>>>>>> +              && ! REG_P (XEXP (*loc, 0)) && ! MEM_P (XEXP (*loc, 0)))
>>>>>> +       /* (subreg (operator ...) ...) usually inside and is used for
>>>>>> +          mode conversion too.  */
>>>>>> +       loc = &XEXP (*loc, 0);
>>>>>
>>>>> I think the condition should be:
>>>>>
>>>>>        else if (code == SUBREG
>>>>>                 && !OBJECT_P (SUBREG_REG (*loc))
>>>>>                 && subreg_lowpart (*loc))
>>>>>
>>>>> OK with that change, if it works.
>>>>>
>>>> Yes, it works.
>>>> I've submitted the following patch.
>>>>
>>>
>>> It doesn't work right.  I will create a new testcase.
>>>
>>
>>
>
> This patch limits SUBREG to Pmode.  Tested on Linux/x86-64.
> OK to install?
>
> Thanks.

The address in this case is:

(plus:SI (mult:SI (reg/v:SI 223 [orig:154 j ] [154])
        (const_int 8 [0x8]))
    (subreg:SI (plus:DI (reg/f:DI 20 frame)
            (const_int 32 [0x20])) 0))

which after Uros's subreg simplification patch shouldn't be allowed:
the subreg ought to be on the frame register rather than the plus.

The attached patch seems to work for the testcase.  Does it work
more generally?

Richard


gcc/
	* lra-eliminations.c (lra_eliminate_regs_1): Use simplify_gen_subreg
	rather than gen_rtx_SUBREG.
H.J. Lu - Oct. 30, 2012, 11:38 a.m.
On Tue, Oct 30, 2012 at 4:09 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>> On Mon, Oct 29, 2012 at 5:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Oct 29, 2012 at 4:41 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Mon, Oct 29, 2012 at 9:38 AM, Vladimir Makarov
>>>> <vmakarov@redhat.com> wrote:
>>>>> On 12-10-29 12:21 PM, Richard Sandiford wrote:
>>>>>>
>>>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>>>>
>>>>>>>     H.J. in
>>>>>>>
>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55116
>>>>>>>
>>>>>>>     reported an interesting address
>>>>>>>
>>>>>>> (and:DI (subreg:DI (plus:SI (ashift:SI (reg:SI 96 [ glob_vol_int.22 ])
>>>>>>>                   (const_int 2 [0x2]))
>>>>>>>               (symbol_ref:SI ("glob_vol_int_arr") <var_decl
>>>>>>> 0x7ffff03c2720 glob_vol_int_arr>)) 0)
>>>>>>>       (const_int 4294967295 [0xffffffff]))
>>>>>>>
>>>>>>>     which can not be correctly extracted.  Here `and' with `subreg'
>>>>>>> behaves as an address mutation.
>>>>>>>
>>>>>>>     The following patch fixes the problem.
>>>>>>>
>>>>>>> Ok to commit, Richard?
>>>>>>
>>>>>> Heh, I wondered if subregs might still be used like that, and was almost
>>>>>> tempted to add them just in case.
>>>>>>
>>>>>> I think this particular case is really a failed canonicalisation and that:
>>>>>>
>>>>>>     (and:DI (subreg:DI (foo:SI ...) 0) (const_int 0xffffffff))
>>>>>>
>>>>>> ought to be:
>>>>>>
>>>>>>     (zero_extend:DI (foo:SI ...))
>>>>>
>>>>> Yes, that was my thought too.
>>>>>
>>>>>> But I know I've approved MIPS patches to accept
>>>>>> (and:DI ... (const_int 0xffffffff)) as an alternative.
>>>>>>
>>>>>>> Index: rtlanal.c
>>>>>>> ===================================================================
>>>>>>> --- rtlanal.c   (revision 192942)
>>>>>>> +++ rtlanal.c   (working copy)
>>>>>>> @@ -5459,6 +5459,11 @@ strip_address_mutations (rtx *loc, enum
>>>>>>>          else if (code == AND && CONST_INT_P (XEXP (*loc, 1)))
>>>>>>>           /* (and ... (const_int -X)) is used to align to X bytes.  */
>>>>>>>           loc = &XEXP (*loc, 0);
>>>>>>> +      else if (code == SUBREG
>>>>>>> +              && ! REG_P (XEXP (*loc, 0)) && ! MEM_P (XEXP (*loc, 0)))
>>>>>>> +       /* (subreg (operator ...) ...) usually inside and is used for
>>>>>>> +          mode conversion too.  */
>>>>>>> +       loc = &XEXP (*loc, 0);
>>>>>>
>>>>>> I think the condition should be:
>>>>>>
>>>>>>        else if (code == SUBREG
>>>>>>                 && !OBJECT_P (SUBREG_REG (*loc))
>>>>>>                 && subreg_lowpart (*loc))
>>>>>>
>>>>>> OK with that change, if it works.
>>>>>>
>>>>> Yes, it works.
>>>>> I've submitted the following patch.
>>>>>
>>>>
>>>> It doesn't work right.  I will create a new testcase.
>>>>
>>>
>>>
>>
>> This patch limits SUBREG to Pmode.  Tested on Linux/x86-64.
>> OK to install?
>>
>> Thanks.
>
> The address in this case is:
>
> (plus:SI (mult:SI (reg/v:SI 223 [orig:154 j ] [154])
>         (const_int 8 [0x8]))
>     (subreg:SI (plus:DI (reg/f:DI 20 frame)
>             (const_int 32 [0x20])) 0))
>
> which after Uros's subreg simplification patch shouldn't be allowed:
> the subreg ought to be on the frame register rather than the plus.
>
> The attached patch seems to work for the testcase.  Does it work
> more generally?
>
> Richard
>
>
> gcc/
>         * lra-eliminations.c (lra_eliminate_regs_1): Use simplify_gen_subreg
>         rather than gen_rtx_SUBREG.
>
> Index: gcc/lra-eliminations.c
> ===================================================================
> --- gcc/lra-eliminations.c      (revision 192983)
> +++ gcc/lra-eliminations.c      (working copy)
> @@ -550,7 +550,8 @@
>               return x;
>             }
>           else
> -           return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
> +           return simplify_gen_subreg (GET_MODE (x), new_rtx,
> +                                       GET_MODE (new_rtx), SUBREG_BYTE (x));
>         }
>
>        return x;

I am running the full test.

Thanks.
H.J. Lu - Oct. 30, 2012, 1:35 p.m.
On Tue, Oct 30, 2012 at 4:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 4:09 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>>> On Mon, Oct 29, 2012 at 5:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Mon, Oct 29, 2012 at 4:41 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Mon, Oct 29, 2012 at 9:38 AM, Vladimir Makarov
>>>>> <vmakarov@redhat.com> wrote:
>>>>>> On 12-10-29 12:21 PM, Richard Sandiford wrote:
>>>>>>>
>>>>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>>>>>
>>>>>>>>     H.J. in
>>>>>>>>
>>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55116
>>>>>>>>
>>>>>>>>     reported an interesting address
>>>>>>>>
>>>>>>>> (and:DI (subreg:DI (plus:SI (ashift:SI (reg:SI 96 [ glob_vol_int.22 ])
>>>>>>>>                   (const_int 2 [0x2]))
>>>>>>>>               (symbol_ref:SI ("glob_vol_int_arr") <var_decl
>>>>>>>> 0x7ffff03c2720 glob_vol_int_arr>)) 0)
>>>>>>>>       (const_int 4294967295 [0xffffffff]))
>>>>>>>>
>>>>>>>>     which can not be correctly extracted.  Here `and' with `subreg'
>>>>>>>> behaves as an address mutation.
>>>>>>>>
>>>>>>>>     The following patch fixes the problem.
>>>>>>>>
>>>>>>>> Ok to commit, Richard?
>>>>>>>
>>>>>>> Heh, I wondered if subregs might still be used like that, and was almost
>>>>>>> tempted to add them just in case.
>>>>>>>
>>>>>>> I think this particular case is really a failed canonicalisation and that:
>>>>>>>
>>>>>>>     (and:DI (subreg:DI (foo:SI ...) 0) (const_int 0xffffffff))
>>>>>>>
>>>>>>> ought to be:
>>>>>>>
>>>>>>>     (zero_extend:DI (foo:SI ...))
>>>>>>
>>>>>> Yes, that was my thought too.
>>>>>>
>>>>>>> But I know I've approved MIPS patches to accept
>>>>>>> (and:DI ... (const_int 0xffffffff)) as an alternative.
>>>>>>>
>>>>>>>> Index: rtlanal.c
>>>>>>>> ===================================================================
>>>>>>>> --- rtlanal.c   (revision 192942)
>>>>>>>> +++ rtlanal.c   (working copy)
>>>>>>>> @@ -5459,6 +5459,11 @@ strip_address_mutations (rtx *loc, enum
>>>>>>>>          else if (code == AND && CONST_INT_P (XEXP (*loc, 1)))
>>>>>>>>           /* (and ... (const_int -X)) is used to align to X bytes.  */
>>>>>>>>           loc = &XEXP (*loc, 0);
>>>>>>>> +      else if (code == SUBREG
>>>>>>>> +              && ! REG_P (XEXP (*loc, 0)) && ! MEM_P (XEXP (*loc, 0)))
>>>>>>>> +       /* (subreg (operator ...) ...) usually inside and is used for
>>>>>>>> +          mode conversion too.  */
>>>>>>>> +       loc = &XEXP (*loc, 0);
>>>>>>>
>>>>>>> I think the condition should be:
>>>>>>>
>>>>>>>        else if (code == SUBREG
>>>>>>>                 && !OBJECT_P (SUBREG_REG (*loc))
>>>>>>>                 && subreg_lowpart (*loc))
>>>>>>>
>>>>>>> OK with that change, if it works.
>>>>>>>
>>>>>> Yes, it works.
>>>>>> I've submitted the following patch.
>>>>>>
>>>>>
>>>>> It doesn't work right.  I will create a new testcase.
>>>>>
>>>>
>>>>
>>>
>>> This patch limits SUBREG to Pmode.  Tested on Linux/x86-64.
>>> OK to install?
>>>
>>> Thanks.
>>
>> The address in this case is:
>>
>> (plus:SI (mult:SI (reg/v:SI 223 [orig:154 j ] [154])
>>         (const_int 8 [0x8]))
>>     (subreg:SI (plus:DI (reg/f:DI 20 frame)
>>             (const_int 32 [0x20])) 0))
>>
>> which after Uros's subreg simplification patch shouldn't be allowed:
>> the subreg ought to be on the frame register rather than the plus.
>>
>> The attached patch seems to work for the testcase.  Does it work
>> more generally?
>>
>> Richard
>>
>>
>> gcc/
>>         * lra-eliminations.c (lra_eliminate_regs_1): Use simplify_gen_subreg
>>         rather than gen_rtx_SUBREG.
>>
>> Index: gcc/lra-eliminations.c
>> ===================================================================
>> --- gcc/lra-eliminations.c      (revision 192983)
>> +++ gcc/lra-eliminations.c      (working copy)
>> @@ -550,7 +550,8 @@
>>               return x;
>>             }
>>           else
>> -           return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
>> +           return simplify_gen_subreg (GET_MODE (x), new_rtx,
>> +                                       GET_MODE (new_rtx), SUBREG_BYTE (x));
>>         }
>>
>>        return x;
>
> I am running the full test.
>

It works.  Can you check in your patch?  I will check in
my testcase.

Thanks.
Richard Sandiford - Oct. 30, 2012, 1:39 p.m.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Tue, Oct 30, 2012 at 4:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Oct 30, 2012 at 4:09 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> The address in this case is:
>>>
>>> (plus:SI (mult:SI (reg/v:SI 223 [orig:154 j ] [154])
>>>         (const_int 8 [0x8]))
>>>     (subreg:SI (plus:DI (reg/f:DI 20 frame)
>>>             (const_int 32 [0x20])) 0))
>>>
>>> which after Uros's subreg simplification patch shouldn't be allowed:
>>> the subreg ought to be on the frame register rather than the plus.
>>>
>>> The attached patch seems to work for the testcase.  Does it work
>>> more generally?
>>>
>>> Richard
>>>
>>>
>>> gcc/
>>>         * lra-eliminations.c (lra_eliminate_regs_1): Use simplify_gen_subreg
>>>         rather than gen_rtx_SUBREG.
>>>
>>> Index: gcc/lra-eliminations.c
>>> ===================================================================
>>> --- gcc/lra-eliminations.c      (revision 192983)
>>> +++ gcc/lra-eliminations.c      (working copy)
>>> @@ -550,7 +550,8 @@
>>>               return x;
>>>             }
>>>           else
>>> -           return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
>>> +           return simplify_gen_subreg (GET_MODE (x), new_rtx,
>>> +                                       GET_MODE (new_rtx), SUBREG_BYTE (x));
>>>         }
>>>
>>>        return x;
>>
>> I am running the full test.
>>
>
> It works.  Can you check in your patch?  I will check in
> my testcase.

Thanks.  Vlad, is the patch OK?

Richard
Vladimir Makarov - Oct. 30, 2012, 2:20 p.m.
On 10/30/2012 09:39 AM, Richard Sandiford wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>> On Tue, Oct 30, 2012 at 4:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Oct 30, 2012 at 4:09 AM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>> The address in this case is:
>>>>
>>>> (plus:SI (mult:SI (reg/v:SI 223 [orig:154 j ] [154])
>>>>          (const_int 8 [0x8]))
>>>>      (subreg:SI (plus:DI (reg/f:DI 20 frame)
>>>>              (const_int 32 [0x20])) 0))
>>>>
>>>> which after Uros's subreg simplification patch shouldn't be allowed:
>>>> the subreg ought to be on the frame register rather than the plus.
>>>>
>>>> The attached patch seems to work for the testcase.  Does it work
>>>> more generally?
>>>>
>>>> Richard
>>>>
>>>>
>>>> gcc/
>>>>          * lra-eliminations.c (lra_eliminate_regs_1): Use simplify_gen_subreg
>>>>          rather than gen_rtx_SUBREG.
>>>>
>>>> Index: gcc/lra-eliminations.c
>>>> ===================================================================
>>>> --- gcc/lra-eliminations.c      (revision 192983)
>>>> +++ gcc/lra-eliminations.c      (working copy)
>>>> @@ -550,7 +550,8 @@
>>>>                return x;
>>>>              }
>>>>            else
>>>> -           return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
>>>> +           return simplify_gen_subreg (GET_MODE (x), new_rtx,
>>>> +                                       GET_MODE (new_rtx), SUBREG_BYTE (x));
>>>>          }
>>>>
>>>>         return x;
>>> I am running the full test.
>>>
>> It works.  Can you check in your patch?  I will check in
>> my testcase.
> Thanks.  Vlad, is the patch OK?
>
Sure.  Thanks.

Patch

Index: gcc/lra-eliminations.c
===================================================================
--- gcc/lra-eliminations.c	(revision 192983)
+++ gcc/lra-eliminations.c	(working copy)
@@ -550,7 +550,8 @@ 
 	      return x;
 	    }
 	  else
-	    return gen_rtx_SUBREG (GET_MODE (x), new_rtx, SUBREG_BYTE (x));
+	    return simplify_gen_subreg (GET_MODE (x), new_rtx,
+					GET_MODE (new_rtx), SUBREG_BYTE (x));
 	}
 
       return x;