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

[Richard Earnshaw]

On 05/05/15 15:06, Richard Biener wrote:
> On May 5, 2015 2:49:55 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> 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.
> 
> No - the issue is you are looking at the type of the value at all, instead of at the type of the formal.
> 

And I would argue that passing the type of the acutal is stupid in all
cases except when the type of the formal does not exist.  It's just
asking for problems like these.

R.

> Richard.
> 
>> R.
>>
>>> ...
>>>
>>> R
>>>
> 
>