PATCH: PR target/46195: r165965 regresses i386 darwin

On Fri, Oct 29, 2010 at 10:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Oct 29, 2010 at 7:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> 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?
>>
>> Because contains_aligned_value_p is wrong, which causes:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44948
>> We use contains_aligned_value_p to compute the old/wrong
>> value only when we warn psABI changes.
>
> I see. So, please rename existing contains_aligned_value_p to
> ix86_compat_aligned_value_p, and please add a big comment explaining
> that the function is obsolete and is only be used for compatibility
> with previous versions. For consistency,
> ix86_old_function_arg_boundary should also be renamed to
> ix86_compat_function_arg_boundary and similar comment about obsolete
> compat function should be added there.
>
> New function can then be named ix86_contains_aligned_value_p, since it
> substitutes existing obsolete 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;
>>
>>
>> contains_aligned_value_p may give the wrong answer
>> and lead to:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44948
>
> Yeah, new function should be used here, but please keep the form above
> for clarity (I hope it is correct, it is untested).
>

Here is the updated patch  Jack, could you please test it on Darwin?
OK for trunk if there are no regressions on Linux and Darwin?

Thanks.

On Fri, Oct 29, 2010 at 10:42:22AM -0700, H.J. Lu wrote:
> 
> Here is the updated patch  Jack, could you please test it on Darwin?
> OK for trunk if there are no regressions on Linux and Darwin?
> 
> Thanks.
> 

H.J.,
   This latest version of the patch still eliminates all of the regressions
from PR 46195 on x86_64-apple-darwin10.
           Jack

> -- 
> H.J.
> ---
> 2010-10-29  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	PR target/46195
> 	* config/i386/i386.c (contains_aligned_value_p): Renamed to ...
> 	(ix86_compat_aligned_value_p): This.
> 	(ix86_old_function_arg_boundary): Updated.
> 	(ix86_contains_aligned_value_p): New.
> 	(ix86_function_arg_boundary): Align long double parameters on
> 	stack to 4byte in 32bit.

> 2010-10-29  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	PR target/46195
> 	* config/i386/i386.c (contains_aligned_value_p): Renamed to ...
> 	(ix86_compat_aligned_value_p): This.
> 	(ix86_old_function_arg_boundary): Updated.
> 	(ix86_contains_aligned_value_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..4fa038e 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -6952,10 +6952,12 @@ ix86_pass_by_reference (CUMULATIVE_ARGS *cum ATTRIBUTE_UNUSED,
>    return 0;
>  }
>  
> -/* Return true when TYPE should be 128bit aligned for 32bit argument passing
> -   ABI.  */
> +/* Return true when TYPE should be 128bit aligned for 32bit argument
> +   passing ABI.  XXX: This function is obsolete and is only used for
> +   checking psABI compatibility with previous versions of GCC.  */
> +
>  static bool
> -contains_aligned_value_p (const_tree type)
> +ix86_compat_aligned_value_p (const_tree type)
>  {
>    enum machine_mode mode = TYPE_MODE (type);
>    if (((TARGET_SSE && SSE_REG_MODE_P (mode))
> @@ -6982,7 +6984,7 @@ contains_aligned_value_p (const_tree type)
>  	    for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>  	      {
>  		if (TREE_CODE (field) == FIELD_DECL
> -		    && contains_aligned_value_p (TREE_TYPE (field)))
> +		    && ix86_compat_aligned_value_p (TREE_TYPE (field)))
>  		  return true;
>  	      }
>  	    break;
> @@ -6990,7 +6992,7 @@ contains_aligned_value_p (const_tree type)
>  
>  	case ARRAY_TYPE:
>  	  /* Just for use if some languages passes arrays by value.  */
> -	  if (contains_aligned_value_p (TREE_TYPE (type)))
> +	  if (ix86_compat_aligned_value_p (TREE_TYPE (type)))
>  	    return true;
>  	  break;
>  
> @@ -7023,7 +7025,7 @@ ix86_old_function_arg_boundary (enum machine_mode mode, const_tree type,
>  	}
>        else
>  	{
> -	  if (!contains_aligned_value_p (type))
> +	  if (!ix86_compat_aligned_value_p (type))
>  	    align = PARM_BOUNDARY;
>  	}
>      }
> @@ -7032,6 +7034,59 @@ ix86_old_function_arg_boundary (enum machine_mode mode, const_tree type,
>    return align;
>  }
>  
> +/* Return true when TYPE should be 128bit aligned for 32bit argument
> +   passing ABI.  */
> +
> +static bool
> +ix86_contains_aligned_value_p (const_tree type)
> +{
> +  enum machine_mode mode = TYPE_MODE (type);
> +
> +  if (mode == XFmode || mode == XCmode)
> +    return false;
> +
> +  if (TYPE_ALIGN (type) < 128)
> +    return false;
> +
> +  if (!AGGREGATE_TYPE_P (type))
> +    return TYPE_ALIGN (type) >= 128;
> +  else
> +    {
> +      /* Walk the aggregates recursively.  */
> +      switch (TREE_CODE (type))
> +	{
> +	case RECORD_TYPE:
> +	case UNION_TYPE:
> +	case QUAL_UNION_TYPE:
> +	  {
> +	    tree field;
> +
> +	    /* Walk all the structure fields.  */
> +	    for (field = TYPE_FIELDS (type);
> +		 field;
> +		 field = DECL_CHAIN (field))
> +	      {
> +		if (TREE_CODE (field) == FIELD_DECL
> +		    && ix86_contains_aligned_value_p (TREE_TYPE (field)))
> +		  return true;
> +	      }
> +	    break;
> +	  }
> +
> +	case ARRAY_TYPE:
> +	  /* Just for use if some languages passes arrays by value.  */
> +	  if (ix86_contains_aligned_value_p (TREE_TYPE (type)))
> +	    return true;
> +	  break;
> +
> +	default:
> +	  gcc_unreachable ();
> +	}
> +    }
> +
> +  return false;
> +}
> +
>  /* Gives the alignment boundary, in bits, of an argument with the
>     specified mode and type.  */
>  
> @@ -7055,8 +7110,20 @@ ix86_function_arg_boundary (enum machine_mode mode, const_tree type)
>        static bool warned;
>        int saved_align = align;
>  
> -      if (!TARGET_64BIT && align < 128)
> -	align = PARM_BOUNDARY;
> +      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 (!ix86_contains_aligned_value_p (type))
> +	    align = PARM_BOUNDARY;
> +
> +	  if (align < 128)
> +	    align = PARM_BOUNDARY;
> +	}
>  
>        if (warn_psabi
>  	  && !warned

On Sat, Oct 30, 2010 at 3:06 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
> On Fri, Oct 29, 2010 at 10:42:22AM -0700, H.J. Lu wrote:
>>
>> Here is the updated patch  Jack, could you please test it on Darwin?
>> OK for trunk if there are no regressions on Linux and Darwin?
>>
>> Thanks.
>>
>
> H.J.,
>   This latest version of the patch still eliminates all of the regressions
> from PR 46195 on x86_64-apple-darwin10.

Thanks!

H.J., please also rename ix86_old_function_arg_boundary to
ix86_compat_function_arg_boundary and add the comment for obsolete
compat function there.

OK for mainline with this addition.

Thanks,
Uros.

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

> Here is the updated patch  Jack, could you please test it on Darwin?
> OK for trunk if there are no regressions on Linux and Darwin?
>
> Thanks.
>
> --
> H.J.
> ---
> 2010-10-29  H.J. Lu  <hongjiu.lu@intel.com>
>
>        PR target/46195
>        * config/i386/i386.c (contains_aligned_value_p): Renamed to ...
>        (ix86_compat_aligned_value_p): This.
>        (ix86_old_function_arg_boundary): Updated.
>        (ix86_contains_aligned_value_p): New.
>        (ix86_function_arg_boundary): Align long double parameters on
>        stack to 4byte in 32bit.
>

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

Please avoid negative tests. Just switch the arms of the "if" for this case.

Uros.