On Wed, Aug 8, 2012 at 1:08 AM, Richard Sandiford <rdsandif...@googlemail.com> wrote: > "H.J. Lu" <hjl.to...@gmail.com> writes: >> diff --git a/gcc/combine.c b/gcc/combine.c >> index a07c046..b9a3589 100644 >> --- a/gcc/combine.c >> +++ b/gcc/combine.c >> @@ -10784,12 +10784,30 @@ gen_lowpart_for_combine (enum machine_mode omode, >> rtx >> x) >> if (omode == imode) >> return x; >> >> - /* Return identity if this is a CONST or symbolic reference. */ >> - if (omode == Pmode >> - && (GET_CODE (x) == CONST >> - || GET_CODE (x) == SYMBOL_REF >> - || GET_CODE (x) == LABEL_REF)) >> - return x; >> + if (omode == Pmode) >> + { >> + /* Return identity if this is a symbolic reference. */ >> + if (GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF) >> + return x; >> + >> + /* Return identity for CONST unless this is a PLUS of 2 constant >> + operands. */ >> + if (GET_CODE (x) == CONST) >> + { >> + rtx y = XEXP (x, 0); >> + if (GET_CODE (y) == PLUS >> + && ((CONST_INT_P (XEXP (y, 1)) >> + && (GET_CODE (XEXP (y, 0)) == CONST >> + || GET_CODE (XEXP (y, 0)) == SYMBOL_REF >> + || GET_CODE (XEXP (y, 0)) == LABEL_REF)) >> + || (CONST_INT_P (XEXP (y, 1)) >> + && (GET_CODE (XEXP (y, 0)) == CONST >> + || GET_CODE (XEXP (y, 0)) == SYMBOL_REF >> + || GET_CODE (XEXP (y, 0)) == LABEL_REF)))) >> + goto fail; >> + return x; >> + } >> + } >> >> /* We can only support MODE being wider than a word if X is a >> constant integer or has a mode the same size. */ >> >> works for the testcase. > > My point was that the "return x" is always wrong. Whenever we return x > here, we know we're returning something in a different mode from the one > that the caller wanted. Returning a Pmode LABEL_REF might not trigger > that plus_constant assert, but it's still wrong. > > It looks like this came from the mips-rewrite branch: > > 2003-03-13 Richard Sandiford <rsand...@redhat.com> > > * combine.c (gen_lowpart_for_combine): Treat the lowpart Pmode of > a CONST as identity. Check the return value of gen_lowpart_common. > > so I can categorically confirm that the person who wrote it didn't > know what they were doing. It also means that this case was motivated > by an experiment to make Pmode == DImode for n32, which we ended up > discarding because it produced worse code. > > If this case really is important, we might consider using > convert_memory_address (Pmode, x) instead. I'm not sure whether > that would be right for targets with address spaces though, because > we don't know which address space is associated with the address. > Hopefully someone who knows address spaces can comment. > > If it is correct, then it should probably go in gen_lowpart_common > rather than gen_lowpart_for_combine. > > But given how few people have hit this, it doesn't look like a > particularly important attempted optimisation. I'll pre-approve > a patch that undoes my mistake and simply removes: > > /* Return identity if this is a CONST or symbolic reference. */ > if (omode == Pmode > && (GET_CODE (x) == CONST > || GET_CODE (x) == SYMBOL_REF > || GET_CODE (x) == LABEL_REF)) > return x; > > Richard
This is the patch I checked in. Thanks. -- H.J. --- gcc/ 2012-08-08 Richard Sandiford <rdsandif...@googlemail.com> H.J. Lu <hongjiu...@intel.com> PR rtl-optimization/54157 * combine.c (gen_lowpart_for_combine): Don't return identity for CONST or symbolic reference. gcc/testsuite/ 2012-08-08 H.J. Lu <hongjiu...@intel.com> PR rtl-optimization/54157 * gcc.target/i386/pr54157.c: New file. diff --git a/gcc/combine.c b/gcc/combine.c index 495e129..2b91eb9 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -10634,13 +10634,6 @@ gen_lowpart_for_combine (enum machine_mode omode, rtx x) if (omode == imode) return x; - /* Return identity if this is a CONST or symbolic reference. */ - if (omode == Pmode - && (GET_CODE (x) == CONST - || GET_CODE (x) == SYMBOL_REF - || GET_CODE (x) == LABEL_REF)) - return x; - /* We can only support MODE being wider than a word if X is a constant integer or has a mode the same size. */ if (GET_MODE_SIZE (omode) > UNITS_PER_WORD diff --git a/gcc/testsuite/gcc.target/i386/pr54157.c b/gcc/testsuite/gcc.target/i386/pr54157.c new file mode 100644 index 0000000..b5c4528 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr54157.c @@ -0,0 +1,21 @@ +/* { dg-do compile { target { ! { ia32 } } } } */ +/* { dg-options "-O2 -mx32 -maddress-mode=long -ftree-vectorize" } */ + +struct s2{ + int n[24 -1][24 -1][24 -1]; +}; + +struct test2{ + struct s2 e; +}; + +struct test2 tmp2[4]; + +void main1 () +{ + int i,j; + + for (i = 0; i < 24 -4; i++) + for (j = 0; j < 24 -4; j++) + tmp2[2].e.n[1][i][j] = 8; +}