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?
Richard.
>R.
