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. -- Meador Inge CodeSourcery / Mentor Embedded http://www.mentor.com/embedded-software
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));