| Message ID | 87390wcj21.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
"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
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.
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;