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

login
register
mail settings
Submitter H.J. Lu
Date Oct. 29, 2010, 5:42 p.m.
Message ID <AANLkTi=cCUgcQ98uz9U7kpa=_xM6V1nTGKqq2O6HgOxp@mail.gmail.com>
Download mbox | patch
Permalink /patch/69615/
State New
Headers show

Comments

H.J. Lu - Oct. 29, 2010, 5:42 p.m.
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.
Jack Howarth - Oct. 30, 2010, 1:06 a.m.
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
Uros Bizjak - Oct. 30, 2010, 7:19 a.m.
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.
Uros Bizjak - Oct. 30, 2010, 7:44 a.m.
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.

Patch

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