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

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

On 05/05/15 13:46, Richard Earnshaw wrote:
> 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?
>>
>> Richard.
>>
>>> R.
>>
>>
> 
> 
> As is
> 
> int foo2 (int a, int b);
> 
> void bar2 (alignedint x)
> {
>   foo2 (1, x);
> }
> 

The real question here is why is TYPE the type of the value, rather than
the type of the formal as expressed by the prototype (or implicit
prototype in the case of variadics or K&R)?  Surely this is the mid-end
passing the wrong information to the back-end.

R.

> ...
> 
> R
>