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.
Uros.
