On 05/05/15 13:37, Richard Biener wrote:
> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw
> <richard.earns...@foss.arm.com> wrote:
>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>> So at least changing arm_needs_doubleword_align for non-aggregates
>> would
>>>>> likely not break anything that hasn't been broken already and would
>> unbreak
>>>>> the majority of cases.
>>>>
>>>> Attached (untested so far). It indeed changes code generated for
>>>> over-aligned va_arg, but as I believe you can't properly pass those
>> in the
>>>> ... caller, this should just fix it so that va_arg handling matches
>> the
>>>> caller (and likewise for callees for named argument passing).
>>>>
>>>>> The following testcase shows that eipa_sra changes alignment even
>> for the
>>>>> aggregates. Change aligned (8) to aligned (4) to see another
>> possibility.
>>>>
>>>> Actually I misread it, for the aggregates esra actually doesn't
>> change
>>>> anything, which is the reason why the testcase doesn't fail.
>>>> The problem with the scalars is that esra first changed it to the
>>>> over-aligned MEM_REFs and then later on eipa_sra used the types of
>> the
>>>> MEM_REFs created by esra.
>>>>
>>>> 2015-05-05 Jakub Jelinek <ja...@redhat.com>
>>>>
>>>> PR target/65956
>>>> * config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>>>> types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>> itself.
>>>>
>>>> * gcc.c-torture/execute/pr65956.c: New test.
>>>>
>>>> --- gcc/config/arm/arm.c.jj 2015-05-04 21:51:42.000000000 +0200
>>>> +++ gcc/config/arm/arm.c 2015-05-05 09:20:52.481693337 +0200
>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>> static bool
>>>> arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>> {
>>>> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>> + if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>> + return true;
>>>
>>> I don't think this is right (though I suspect the existing code has
>> the
>>> same problem). We should only look at mode if there is no type
>>> information. The problem is that GCC has a nasty habit of assigning
>>> real machine modes to things that are really BLKmode and we've run
>> into
>>> several cases where this has royally screwed things up. So for
>>> consistency in the ARM back-end we are careful to only use mode when
>>> type is NULL (=> it's a libcall).
>>>
>>>> + if (type == NULL_TREE)
>>>> + return false;
>>>> + if (AGGREGATE_TYPE_P (type))
>>>> + return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>> + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>> }
>>>>
>>>
>>>
>>>
>>> It ought to be possible to re-order this, though, to
>>>
>>> static bool
>>> arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>> {
>>> - return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>> - || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>> + if (type != NULL_TREE)
>>> + {
>>> + if (AGGREGATE_TYPE_P (type))
>>> + return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>> + return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>> + }
>>> + return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>> }
>>>
>>>
>>> Either way, this would need careful cross-testing against an existing
>>> compiler.
>>>
>>
>> It looks as though either patch would cause ABI incompatibility for
>>
>> typedef int alignedint __attribute__((aligned((8))));
>>
>> int __attribute__((weak)) foo (int a, alignedint b)
>> {return b;}
>>
>> void bar (alignedint x)
>> {
>> foo (1, x);
>> }
>>
>> Where currently gcc uses r2 as the argument register for b in foo.
>
> And for foo (1,2) or an int typed 2nd arg?
>
Yep, looks like that's horribly broken too ;-(
R.
> Richard.
>
>> R.
>
>
