On Wed, Aug 8, 2012 at 6:43 AM, Uros Bizjak <ubiz...@gmail.com> wrote:
> On Wed, Aug 8, 2012 at 3:40 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
>
>> <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.
>
> 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.
