[rs6000,PR,target/87474] fix strncmp expansion with -mno-power8-vector
diff mbox series

Message ID 1d76627b-6690-6da1-99b1-29aab0dbf860@linux.ibm.com
State New
Headers show
Series
  • [rs6000,PR,target/87474] fix strncmp expansion with -mno-power8-vector
Related show

Commit Message

Aaron Sawdey Oct. 2, 2018, 4:09 a.m. UTC
PR/87474 happens because I didn't check that both vector and VSX instructions
were enabled, so insns that are disabled get generated with -mno-power8-vector.

Regstrap passes on ppc64le, ok for trunk?

Thanks!
  Aaron



2018-10-01  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/87474
	* config/rs6000/rs6000-string.c (expand_strn_compare): Check that both
	vector and VSX are enabled.

Comments

Segher Boessenkool Oct. 2, 2018, 8:38 a.m. UTC | #1
On Mon, Oct 01, 2018 at 11:09:44PM -0500, Aaron Sawdey wrote:
> PR/87474 happens because I didn't check that both vector and VSX instructions
> were enabled, so insns that are disabled get generated with -mno-power8-vector.

> 	PR target/87474
> 	* config/rs6000/rs6000-string.c (expand_strn_compare): Check that both
> 	vector and VSX are enabled.

You mean "P8 vector" or similar, I think?


> --- gcc/config/rs6000/rs6000-string.c	(revision 264760)
> +++ gcc/config/rs6000/rs6000-string.c	(working copy)
> @@ -2205,6 +2205,7 @@
>      }
>    else
>      {
> +      /* Implies TARGET_P8_VECTOR here. */

That isn't true as far as I see.


Okay for trunk with improved changelog and that stray line removed.
Thanks!


Segher
Aaron Sawdey Oct. 2, 2018, 1:24 p.m. UTC | #2
On 10/2/18 3:38 AM, Segher Boessenkool wrote:
> On Mon, Oct 01, 2018 at 11:09:44PM -0500, Aaron Sawdey wrote:
>> PR/87474 happens because I didn't check that both vector and VSX instructions
>> were enabled, so insns that are disabled get generated with -mno-power8-vector.
> 
>> 	PR target/87474
>> 	* config/rs6000/rs6000-string.c (expand_strn_compare): Check that both
>> 	vector and VSX are enabled.
> 
> You mean "P8 vector" or similar, I think?

True, it should say TARGET_P[89]_VECTOR.

> 
> 
>> --- gcc/config/rs6000/rs6000-string.c	(revision 264760)
>> +++ gcc/config/rs6000/rs6000-string.c	(working copy)
>> @@ -2205,6 +2205,7 @@
>>      }
>>    else
>>      {
>> +      /* Implies TARGET_P8_VECTOR here. */
> 
> That isn't true as far as I see.

We can only enter emit_final_str_compare_vec() if TARGET_P8_VECTOR is set.
That's the additional check this patch adds. So in this function you can have
both P8 and P9 vector, or just p8 vector. rs6000_option_override_internal()
enforces that P8 vector must be set if P9 vector is set. So in the else here
we know that only p8 vector is set.

> 
> 
> Okay for trunk with improved changelog and that stray line removed.
> Thanks!
> 
> 
> Segher
>
Segher Boessenkool Oct. 2, 2018, 1:27 p.m. UTC | #3
On Tue, Oct 02, 2018 at 08:24:31AM -0500, Aaron Sawdey wrote:
> >> @@ -2205,6 +2205,7 @@
> >>      }
> >>    else
> >>      {
> >> +      /* Implies TARGET_P8_VECTOR here. */
> > 
> > That isn't true as far as I see.
> 
> We can only enter emit_final_str_compare_vec() if TARGET_P8_VECTOR is set.
> That's the additional check this patch adds. So in this function you can have
> both P8 and P9 vector, or just p8 vector. rs6000_option_override_internal()
> enforces that P8 vector must be set if P9 vector is set. So in the else here
> we know that only p8 vector is set.

It's not obvious.  Add an assert, maybe?  Instead of the comment :-)


Segher

Patch
diff mbox series

Index: gcc/config/rs6000/rs6000-string.c
===================================================================
--- gcc/config/rs6000/rs6000-string.c	(revision 264760)
+++ gcc/config/rs6000/rs6000-string.c	(working copy)
@@ -2205,6 +2205,7 @@ 
     }
   else
     {
+      /* Implies TARGET_P8_VECTOR here. */
       rtx diffix = gen_reg_rtx (DImode);
       rtx result_gbbd = gen_reg_rtx (V16QImode);
       /* Since each byte of the input is either 00 or FF, the bytes in
@@ -2313,9 +2314,12 @@ 
   /* Is it OK to use vec/vsx for this. TARGET_VSX means we have at
      least POWER7 but we use TARGET_EFFICIENT_UNALIGNED_VSX which is
      at least POWER8.  That way we can rely on overlapping compares to
-     do the final comparison of less than 16 bytes.  Also I do not want
-     to deal with making this work for 32 bits.  */
-  int use_vec = (bytes >= 16 && !TARGET_32BIT && TARGET_EFFICIENT_UNALIGNED_VSX);
+     do the final comparison of less than 16 bytes.  Also I do not
+     want to deal with making this work for 32 bits.  In addition, we
+     have to make sure that we have at least P8_VECTOR (we don't allow
+     P9_VECTOR without P8_VECTOR).  */
+  int use_vec = (bytes >= 16 && !TARGET_32BIT
+		 && TARGET_EFFICIENT_UNALIGNED_VSX && TARGET_P8_VECTOR);

   if (use_vec)
     required_align = 16;