diff mbox series

Remove setting -mblock-ops-vector-pair on power10.

Message ID Ytj1VQ5aTY+Ap4BW@toto.the-meissners.org
State New, archived
Headers
Series Remove setting -mblock-ops-vector-pair on power10. |

Commit Message

Li, Pan2 via Gcc-patches July 21, 2022, 6:42 a.m. UTC 
  Remove setting -mblock-ops-vector-pair on power10.

Testing has shown that using the load vector pair and store vector pair
instructions for block moves has some performance issues on power10.  This
patch does not set this option by default.  If it is a win in other
machines in the future, this flag can be set in the ISA options.

This patch modifies a previous patch that attempted to do the same thing
but the previous patch was flawed in that it would generate the load
vector pair and store vector pair instructions if we are tuning for a
different machine.

I have tested this patch on power10 systems (with long double set to IEEE
128-bit and also with long double set to IBM 128-bit).  Can I check this patch
into the trunk and back port to older GCC releases?

2022-07-21   Michael Meissner  <meissner@linux.ibm.com>

gcc/

	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
	Do not enable -mblock-ops-vector-pair by default on power10.
---
 gcc/config/rs6000/rs6000.cc | 11 -----------
 1 file changed, 11 deletions(-)

Comments

Segher Boessenkool July 21, 2022, 8:50 p.m. UTC | #1 
On Thu, Jul 21, 2022 at 02:42:29AM -0400, Michael Meissner wrote:
> Testing has shown that using the load vector pair and store vector pair
> instructions for block moves has some performance issues on power10.  This
> patch does not set this option by default.  If it is a win in other
> machines in the future, this flag can be set in the ISA options.

This would make rs6000_isa_flags an even bigger misnomer than it already
is, sigh.

> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
> 	Do not enable -mblock-ops-vector-pair by default on power10.

Do not wrap lines early, especially if that would mean leaving a colon
at the end of a line.  Changelog lines are 80 positions long (including
the leading tab, which counts as eight).

> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4139,17 +4139,6 @@ rs6000_option_override_internal (bool global_init_p)
>  	rs6000_isa_flags &= ~OPTION_MASK_BLOCK_OPS_UNALIGNED_VSX;
>      }
>  
> -  if (!(rs6000_isa_flags_explicit & OPTION_MASK_BLOCK_OPS_VECTOR_PAIR))
> -    {
> -      /* Do not generate lxvp and stxvp on power10 since there are some
> -	 performance issues.  */
> -      if (TARGET_MMA && TARGET_EFFICIENT_UNALIGNED_VSX
> -	  && rs6000_tune != PROCESSOR_POWER10)
> -	rs6000_isa_flags |= OPTION_MASK_BLOCK_OPS_VECTOR_PAIR;
> -      else
> -	rs6000_isa_flags &= ~OPTION_MASK_BLOCK_OPS_VECTOR_PAIR;
> -    }

How does this implement what the changelog says it does?  With what it
does the changelog should instead say to not touch it at all (your patch
also disables the code that disables it!)

It isn't clear what you intended: what your changelog says, or what the
code does.


Segher
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 3ff16b8ae04..667f83b1dfd 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -4139,17 +4139,6 @@  rs6000_option_override_internal (bool global_init_p)
 	rs6000_isa_flags &= ~OPTION_MASK_BLOCK_OPS_UNALIGNED_VSX;
     }
 
-  if (!(rs6000_isa_flags_explicit & OPTION_MASK_BLOCK_OPS_VECTOR_PAIR))
-    {
-      /* Do not generate lxvp and stxvp on power10 since there are some
-	 performance issues.  */
-      if (TARGET_MMA && TARGET_EFFICIENT_UNALIGNED_VSX
-	  && rs6000_tune != PROCESSOR_POWER10)
-	rs6000_isa_flags |= OPTION_MASK_BLOCK_OPS_VECTOR_PAIR;
-      else
-	rs6000_isa_flags &= ~OPTION_MASK_BLOCK_OPS_VECTOR_PAIR;
-    }
-
   /* Use long double size to select the appropriate long double.  We use
      TYPE_PRECISION to differentiate the 3 different long double types.  We map
      128 into the precision used for TFmode.  */