| Message ID | CAMe9rOrtf40ViaoK=W+XVZ543dp8M-w7VM142GYU9C_+DbKATQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 8, 2012 at 3:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > <rdsandiford@googlemail.com> wrote: >> "H.J. Lu" <hjl.tools@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 <rsandifo@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. Probably we need to backport this patch to 4.7, where x32 is -maddress-mode=long by default. Uros.
On Wed, Aug 8, 2012 at 6:43 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Wed, Aug 8, 2012 at 3:40 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >> <rdsandiford@googlemail.com> wrote: >>> "H.J. Lu" <hjl.tools@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 <rsandifo@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. > > Probably we need to backport this patch to 4.7, where x32 is > -maddress-mode=long by default. > It doesn't fail on 4.7 branch since checking mode on PLUS CONST is new on trunk. However, I think it is a correctness issue. Is this OK to backport to 4.7? Thanks.
"H.J. Lu" <hjl.tools@gmail.com> writes: > On Wed, Aug 8, 2012 at 6:43 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> Probably we need to backport this patch to 4.7, where x32 is >> -maddress-mode=long by default. >> > > It doesn't fail on 4.7 branch since checking mode on PLUS CONST > is new on trunk. However, I think it is a correctness issue. Is this > OK to backport to 4.7? Yeah, I agree we should backport it. Richard
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; +}