PR middle-end/52584

PR middle-end/52584

[David Miller]

Richard, I was looking into a testsuite failure on the 4.7 branch
on sparc and I think your fix for 52584 would fix it too.

The problem eminates in gcc.c-torture/execute/vector-shift2.c with -O0

The 4 integer vector shifts get lowered to 2 integer vector shifts,
since that is what the VIS3 shifts are capable of.

However, type_for_widest_vector_mode does not take the signedness into
consideration.  So:

	signed_vector >> unsigned_vector

gets lowered into:

	unsigned_vector >> unsigned_vector

since type_for_widest_vector_mode unconditionally always passes
'1' for "unsignedp", and thus the testcase aborts since we use
a logical shift instead of an arithmetic one.

I came up with the crude fix below, but then I noticed in mainline
the change you made in order to resolve 52584.

I'd like to see this fixed on the 4.7 branch since the problem
generates incorrect code, rather than merely miss an optimization
opportunity.

diff --git a/gcc/tree-vect-generic.c b/gcc/tree-vect-generic.c
index be54710..aa7b774 100644
--- a/gcc/tree-vect-generic.c
+++ b/gcc/tree-vect-generic.c
@@ -476,7 +476,8 @@ expand_vector_operation (gimple_stmt_iterator *gsi, tree type, tree compute_type
    SATP is true for saturating fixed-point types.  */
 
 static tree
-type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp)
+type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp,
+			     int unsignedp)
 {
   enum machine_mode best_mode = VOIDmode, mode;
   int best_nunits = 0;
@@ -508,7 +509,7 @@ type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp)
       if (ALL_FIXED_POINT_MODE_P (best_mode))
 	return lang_hooks.types.type_for_mode (best_mode, satp);
 
-      return lang_hooks.types.type_for_mode (best_mode, 1);
+      return lang_hooks.types.type_for_mode (best_mode, unsignedp);
     }
 }
 
@@ -858,7 +859,8 @@ expand_vector_operations_1 (gimple_stmt_iterator *gsi)
     {
       tree vector_compute_type
         = type_for_widest_vector_mode (TYPE_MODE (TREE_TYPE (type)), op,
-				       TYPE_SATURATING (TREE_TYPE (type)));
+				       TYPE_SATURATING (TREE_TYPE (type)),
+				       TYPE_UNSIGNED (TREE_TYPE (type)));
       if (vector_compute_type != NULL_TREE
 	  && (TYPE_VECTOR_SUBPARTS (vector_compute_type)
 	      < TYPE_VECTOR_SUBPARTS (compute_type))

Re: PR middle-end/52584

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2889 bytes --]

On Thu, 17 May 2012, David Miller wrote:

> 
> Richard, I was looking into a testsuite failure on the 4.7 branch
> on sparc and I think your fix for 52584 would fix it too.
> 
> The problem eminates in gcc.c-torture/execute/vector-shift2.c with -O0
> 
> The 4 integer vector shifts get lowered to 2 integer vector shifts,
> since that is what the VIS3 shifts are capable of.
> 
> However, type_for_widest_vector_mode does not take the signedness into
> consideration.  So:
> 
> 	signed_vector >> unsigned_vector
> 
> gets lowered into:
> 
> 	unsigned_vector >> unsigned_vector
> 
> since type_for_widest_vector_mode unconditionally always passes
> '1' for "unsignedp", and thus the testcase aborts since we use
> a logical shift instead of an arithmetic one.
> 
> I came up with the crude fix below, but then I noticed in mainline
> the change you made in order to resolve 52584.

Backporting that change is fine with me if you can take care to
do the bootstrap & testing on the 4.7 branch.

Thanks,
Richard.

> I'd like to see this fixed on the 4.7 branch since the problem
> generates incorrect code, rather than merely miss an optimization
> opportunity.
> 
> diff --git a/gcc/tree-vect-generic.c b/gcc/tree-vect-generic.c
> index be54710..aa7b774 100644
> --- a/gcc/tree-vect-generic.c
> +++ b/gcc/tree-vect-generic.c
> @@ -476,7 +476,8 @@ expand_vector_operation (gimple_stmt_iterator *gsi, tree type, tree compute_type
>     SATP is true for saturating fixed-point types.  */
>  
>  static tree
> -type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp)
> +type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp,
> +			     int unsignedp)
>  {
>    enum machine_mode best_mode = VOIDmode, mode;
>    int best_nunits = 0;
> @@ -508,7 +509,7 @@ type_for_widest_vector_mode (enum machine_mode inner_mode, optab op, int satp)
>        if (ALL_FIXED_POINT_MODE_P (best_mode))
>  	return lang_hooks.types.type_for_mode (best_mode, satp);
>  
> -      return lang_hooks.types.type_for_mode (best_mode, 1);
> +      return lang_hooks.types.type_for_mode (best_mode, unsignedp);
>      }
>  }
>  
> @@ -858,7 +859,8 @@ expand_vector_operations_1 (gimple_stmt_iterator *gsi)
>      {
>        tree vector_compute_type
>          = type_for_widest_vector_mode (TYPE_MODE (TREE_TYPE (type)), op,
> -				       TYPE_SATURATING (TREE_TYPE (type)));
> +				       TYPE_SATURATING (TREE_TYPE (type)),
> +				       TYPE_UNSIGNED (TREE_TYPE (type)));
>        if (vector_compute_type != NULL_TREE
>  	  && (TYPE_VECTOR_SUBPARTS (vector_compute_type)
>  	      < TYPE_VECTOR_SUBPARTS (compute_type))
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: PR middle-end/52584

[David Miller]

From: Richard Guenther <rguenther@suse.de>
Date: Fri, 18 May 2012 11:48:55 +0200 (CEST)

> On Thu, 17 May 2012, David Miller wrote:
> 
>> 
>> Richard, I was looking into a testsuite failure on the 4.7 branch
>> on sparc and I think your fix for 52584 would fix it too.
>> 
>> The problem eminates in gcc.c-torture/execute/vector-shift2.c with -O0
>> 
>> The 4 integer vector shifts get lowered to 2 integer vector shifts,
>> since that is what the VIS3 shifts are capable of.
>> 
>> However, type_for_widest_vector_mode does not take the signedness into
>> consideration.  So:
>> 
>> 	signed_vector >> unsigned_vector
>> 
>> gets lowered into:
>> 
>> 	unsigned_vector >> unsigned_vector
>> 
>> since type_for_widest_vector_mode unconditionally always passes
>> '1' for "unsignedp", and thus the testcase aborts since we use
>> a logical shift instead of an arithmetic one.
>> 
>> I came up with the crude fix below, but then I noticed in mainline
>> the change you made in order to resolve 52584.
> 
> Backporting that change is fine with me if you can take care to
> do the bootstrap & testing on the 4.7 branch.

Great, I'll start doing that right now.