Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Biener <rguenther at suse dot de>]

On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@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  <jakub@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.