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 <[email protected]>
>>
>> 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.
R.
