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

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 05/05/15 13:37, Richard Biener wrote:
> 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?
> 

Yep, looks like that's horribly broken too ;-(

R.

> Richard.
> 
>> R.
> 
>