| Message ID | 4FB56399.2000900@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
Meador Inge <meadori@codesourcery.com> writes:
> v2 OK? If so, would you mind committing for me? I don't have write access.
Applied, thanks. (BTW, I removed the brackets around the new condition.)
Richard
On Thu, May 17, 2012 at 1:46 PM, Meador Inge <meadori@codesourcery.com> wrote: > On 05/17/2012 03:02 PM, Richard Sandiford wrote: > >> After agonising over this for a couple of days, I think it's probably >> the correct fix. What we're doing now would be valid if the only use of >> equiv_constant(x) were to substitute for x. The name and current use >> of the function obviously require equality though. >> >> Patch is OK, thanks. It might be worth extending fold_rtx and >> equiv_constant to set a flag that says whether the returned value >> is equivalent or simply one of many valid replacement values. >> We'd then be able to replace uses of registers like 142 with 0, >> while not replacing uses of 0 with 142. I don't know how much it >> would buy us though. That kind of thing is probably more of a job >> for fwprop. > > Thanks for the review. > >> Please add UL to the hex constants in the testcase. Not sure whether >> it matters for 16-bit int targets or not, but better safe than sorry :-) > > Fixed. > >> Also, rather than: >> >>> + /* Otherwise see if we already have a constant for the inner REG. >>> + Don't bother with paradoxical subregs because we have no way >>> + of knowing what the upper bytes are. */ >> >> how about: >> >> /* Otherwise see if we already have a constant for the inner REG, >> and if that is enough to calculate an equivalent constant for >> the subreg. Note that the upper bits of paradoxical subregs >> are undefined, so they cannot be said to equal anything. */ > > Sounds good to me. > > v2 OK? If so, would you mind committing for me? I don't have write access. > The test fails on Linux/x86 and Linux/x86-64.
On 05/18/2012 09:16 AM, H.J. Lu wrote: > On Thu, May 17, 2012 at 1:46 PM, Meador Inge <meadori@codesourcery.com> wrote: >> On 05/17/2012 03:02 PM, Richard Sandiford wrote: >> >>> After agonising over this for a couple of days, I think it's probably >>> the correct fix. What we're doing now would be valid if the only use of >>> equiv_constant(x) were to substitute for x. The name and current use >>> of the function obviously require equality though. >>> >>> Patch is OK, thanks. It might be worth extending fold_rtx and >>> equiv_constant to set a flag that says whether the returned value >>> is equivalent or simply one of many valid replacement values. >>> We'd then be able to replace uses of registers like 142 with 0, >>> while not replacing uses of 0 with 142. I don't know how much it >>> would buy us though. That kind of thing is probably more of a job >>> for fwprop. >> >> Thanks for the review. >> >>> Please add UL to the hex constants in the testcase. Not sure whether >>> it matters for 16-bit int targets or not, but better safe than sorry :-) >> >> Fixed. >> >>> Also, rather than: >>> >>>> + /* Otherwise see if we already have a constant for the inner REG. >>>> + Don't bother with paradoxical subregs because we have no way >>>> + of knowing what the upper bytes are. */ >>> >>> how about: >>> >>> /* Otherwise see if we already have a constant for the inner REG, >>> and if that is enough to calculate an equivalent constant for >>> the subreg. Note that the upper bits of paradoxical subregs >>> are undefined, so they cannot be said to equal anything. */ >> >> Sounds good to me. >> >> v2 OK? If so, would you mind committing for me? I don't have write access. >> > > The test fails on Linux/x86 and Linux/x86-64. I will look into it. Thanks.
On 05/18/2012 09:16 AM, H.J. Lu wrote: > On Thu, May 17, 2012 at 1:46 PM, Meador Inge <meadori@codesourcery.com> wrote: >> On 05/17/2012 03:02 PM, Richard Sandiford wrote: >> >>> After agonising over this for a couple of days, I think it's probably >>> the correct fix. What we're doing now would be valid if the only use of >>> equiv_constant(x) were to substitute for x. The name and current use >>> of the function obviously require equality though. >>> >>> Patch is OK, thanks. It might be worth extending fold_rtx and >>> equiv_constant to set a flag that says whether the returned value >>> is equivalent or simply one of many valid replacement values. >>> We'd then be able to replace uses of registers like 142 with 0, >>> while not replacing uses of 0 with 142. I don't know how much it >>> would buy us though. That kind of thing is probably more of a job >>> for fwprop. >> >> Thanks for the review. >> >>> Please add UL to the hex constants in the testcase. Not sure whether >>> it matters for 16-bit int targets or not, but better safe than sorry :-) >> >> Fixed. >> >>> Also, rather than: >>> >>>> + /* Otherwise see if we already have a constant for the inner REG. >>>> + Don't bother with paradoxical subregs because we have no way >>>> + of knowing what the upper bytes are. */ >>> >>> how about: >>> >>> /* Otherwise see if we already have a constant for the inner REG, >>> and if that is enough to calculate an equivalent constant for >>> the subreg. Note that the upper bits of paradoxical subregs >>> are undefined, so they cannot be said to equal anything. */ >> >> Sounds good to me. >> >> v2 OK? If so, would you mind committing for me? I don't have write access. >> > > The test fails on Linux/x86 and Linux/x86-64. Oh, Richard Guenther already fixed this (thanks!): http://gcc.gnu.org/viewcvs?view=revision&revision=187654
Index: testsuite/gcc.dg/pr53352.c =================================================================== --- testsuite/gcc.dg/pr53352.c (revision 0) +++ testsuite/gcc.dg/pr53352.c (revision 0) @@ -0,0 +1,41 @@ +/* { dg-do run } */ +/* { dg-options "-O1" } */ + +#include <stdlib.h> + +typedef union +{ + struct + { + unsigned char a; + unsigned char b; + unsigned char c; + unsigned char d; + } parts; + unsigned long whole; +} T; + +T *g_t; + +void bar (unsigned long x) +{ + if (x != 0) + abort (); +} + +int main () +{ + T one; + T two; + T tmp1, tmp2; + + one.whole = 0xFFE0E0E0UL; + two.whole = 0xFF000000UL; + tmp1.parts = two.parts; + tmp2.parts = one.parts; + tmp2.parts.c = tmp1.parts.c; + one.parts = tmp2.parts; + + g_t = &one; + bar (0); +} Index: cse.c =================================================================== --- cse.c (revision 187470) +++ cse.c (working copy) @@ -3786,8 +3786,12 @@ equiv_constant (rtx x) } } - /* Otherwise see if we already have a constant for the inner REG. */ + /* Otherwise see if we already have a constant for the inner REG, + and if that is enough to calculate an equivalent constant for + the subreg. Note that the upper bits of paradoxical subregs + are undefined, so they cannot be said to equal anything. */ if (REG_P (SUBREG_REG (x)) + && (GET_MODE_SIZE (mode) <= GET_MODE_SIZE (imode)) && (new_rtx = equiv_constant (SUBREG_REG (x))) != 0) return simplify_subreg (mode, new_rtx, imode, SUBREG_BYTE (x));