Re: PATCH: PR target/46195: r165965 regresses i386 darwin

[Uros Bizjak <ubizjak at gmail dot com>]

On Fri, Oct 29, 2010 at 1:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> On Darwin, although long double is aligned to 16byte, it is aligned to
>>> 4byte when passed on stack.
>>>
>>> Jack, please try this on Darwin. OK for trunk if there are no
>>> regressions on Darwin and Linux?
>>>
>>> Thanks.
>>>
>>>
>>> H.J.
>>> ---
>>> 2010-10-28 ?H.J. Lu ?<hongjiu.lu@intel.com>
>>>
>>> ? ? ? ?PR target/46195
>>> ? ? ? ?* config/i386/i386.c (bigger_than_4byte_aligned_p): New.
>>> ? ? ? ?(ix86_function_arg_boundary): Align long double parameters on
>>> ? ? ? ?stack to 4byte in 32bit.
>>>
>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>>> index f2bd705..02650df 100644
>>> --- a/gcc/config/i386/i386.c
>>> +++ b/gcc/config/i386/i386.c
>>> @@ -7032,6 +7032,59 @@ ix86_old_function_arg_boundary (enum machine_mode mode, const_tree type,
>>> ? return align;
>>> ?}
>>>
>>> +/* Return true when aggregate TYPE should be aligned at bigger than
>>> + ? 4byte for 32bit argument passing ABI. ?*/
>>> +
>>> +static bool
>>> +bigger_than_4byte_aligned_p (const_tree type)
>>
>> I propose to change the function prototype and name to:
>>
>> int ix86_alignment_needed (const_tree_type)
>>
>> and this function will return alignment (in bits), as is customary in gcc.
>
> This function is a simplified version of contains_aligned_value_p,
> which should return true/false.

It is not simplified, only the top condition is changed ...

-  if (((TARGET_SSE && SSE_REG_MODE_P (mode))
-       || mode == TDmode
-       || mode == TFmode
-       || mode == TCmode)
-      && (!TYPE_USER_ALIGN (type) || TYPE_ALIGN (type) > 128))
-    return true;
+  if (mode == XFmode || mode == XCmode)
+    return false;

... and else was added:

  if (AGGREGATE_TYPE_P (type))
    {
...
    }
+  else
+    return TYPE_ALIGN (type) >= 128;

Why can't this be part of existing contains_aligned_value_p?

> +/* Return true when aggregate TYPE should be aligned at 128bit for
> +   32bit argument passing ABI.  */

According to the comment, we already have contains_aligned_value_p for this.

>>> - ? ? ?if (!TARGET_64BIT && align < 128)
>>> + ? ? ?/* i386 ABI defines all arguments to be 4 byte aligned. ?Even if
>>> + ? ? ? ?long double is aligned to 16 byte, we always align it at 4
>>> + ? ? ? ?byte, whether it is a scalar or the part of aggregate, when
>>> + ? ? ? ?passed as function argument. ?*/
>>> + ? ? ?if (!TARGET_64BIT
>>> + ? ? ? ? && (align < 128
>>> + ? ? ? ? ? ? || (align == 128
>>> + ? ? ? ? ? ? ? ? && (mode == XFmode
>>> + ? ? ? ? ? ? ? ? ? ? || mode == XCmode
>>> + ? ? ? ? ? ? ? ? ? ? || (type
>>> + ? ? ? ? ? ? ? ? ? ? ? ? && AGGREGATE_TYPE_P (type)
>>> + ? ? ? ? ? ? ? ? ? ? ? ? && !bigger_than_4byte_aligned_p (type))))))
>>> ? ? ? ?align = PARM_BOUNDARY;
>>
>> There is something wrong with the usage of utility function if it has
>> to be surrounded by soo much extra checks. I propose to change all
>> this to:
>>
>> align = ix86_alignment_needed (type)
>>
>> where all the knowledge of type alignment will be moved into the
>> ix86_alignment_needed function.
>
> We have to check mode since type may be NULL. Long double
> is a very special case for 32bit psABI where normal alignment of a
> type/mode != its alignment when passed on stack. ?So we have
> to check when normal alignment is 128 with the alignment of mode/type
> is determined by long double.

I think that following code is much more readable.

  if (!TARGET_64BIT)
    {
      /* i386 ABI defines XFmode arguments to be 4 byte aligned.  */
      if (!type)
	{
	  if (mode == XFmode || mode == XCmode)
	    align = PARM_BOUNDARY;
	}
      else
	{
	  if (!contains_aligned_value_p (type))
	    align = PARM_BOUNDARY;
	}

      if (align < 128)
        align = PARM_BOUNDARY;
    }

Uros.