Patchwork PATCH: PR rtl-optimization/54157: [x32] -maddress-mode=long failures

login
register
mail settings
Submitter H.J. Lu
Date Aug. 8, 2012, 1:40 p.m.
Message ID <CAMe9rOrtf40ViaoK=W+XVZ543dp8M-w7VM142GYU9C_+DbKATQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/175930/
State New
Headers show

Comments

H.J. Lu - Aug. 8, 2012, 1:40 p.m.
On Wed, Aug 8, 2012 at 1:08 AM, Richard Sandiford
<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.

Thanks.
Uros Bizjak - Aug. 8, 2012, 1:43 p.m.
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.
H.J. Lu - Aug. 8, 2012, 1:50 p.m.
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.
Richard Sandiford - Aug. 8, 2012, 3:11 p.m.
"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

Patch

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;
+}