[PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Peter Bergner]

This is a cleanup patch in preparation to fixing the real bug in PR101865.
TARGET_DIRECT_MOVE is redundant with TARGET_P8_VECTOR, so alias it to that.
Also replace all usages of OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR
and delete the now dead mask.

This passed bootstrap and retesting on powerpc64le-linux with no regressions.
Ok for trunk?

Eventually we'll want to backport this along with the follow-on patch that
actually fixes PR101865.

Peter


gcc/
	PR target/101865
	* config/rs6000/rs6000.h (TARGET_DIRECT_MOVE): Define.
	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Replace
	OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR.  Delete redundant
	OPTION_MASK_DIRECT_MOVE usage.  Delete TARGET_DIRECT_MOVE dead code.
	(rs6000_opt_masks): Neuter the "direct-move" option.
	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Replace
	OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR.  Delete useless
	comment.
	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER): Delete
	OPTION_MASK_DIRECT_MOVE.
	(OTHER_VSX_VECTOR_MASKS): Likewise.
	(POWERPC_MASKS): Likewise.
	* config/rs6000/rs6000.opt (mno-direct-move): New.
	(mdirect-move): Remove Mask and Var.


diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 68bc45d65ba..77d045c9f6e 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -471,6 +471,8 @@ extern int rs6000_vector_align[];
 #define TARGET_EXTSWSLI	(TARGET_MODULO && TARGET_POWERPC64)
 #define TARGET_MADDLD	TARGET_MODULO
 
+/* TARGET_DIRECT_MOVE is redundant to TARGET_P8_VECTOR, so alias it to that.  */
+#define TARGET_DIRECT_MOVE	TARGET_P8_VECTOR
 #define TARGET_XSCVDPSPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
 #define TARGET_XSCVSPDPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
 #define TARGET_VADDUQM		(TARGET_P8_VECTOR && TARGET_POWERPC64)
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 6ba9df4f02e..c241371147c 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3811,7 +3811,7 @@ rs6000_option_override_internal (bool global_init_p)
      Testing for direct_move matches power8 and later.  */
   if (!BYTES_BIG_ENDIAN
       && !(processor_target_table[tune_index].target_enable
-	   & OPTION_MASK_DIRECT_MOVE))
+	   & OPTION_MASK_P8_VECTOR))
     rs6000_isa_flags |= ~rs6000_isa_flags_explicit & OPTION_MASK_STRICT_ALIGN;
 
   /* Add some warnings for VSX.  */
@@ -3853,8 +3853,7 @@ rs6000_option_override_internal (bool global_init_p)
       && (rs6000_isa_flags_explicit & (OPTION_MASK_SOFT_FLOAT
 				       | OPTION_MASK_ALTIVEC
 				       | OPTION_MASK_VSX)) != 0)
-    rs6000_isa_flags &= ~((OPTION_MASK_P8_VECTOR | OPTION_MASK_CRYPTO
-			   | OPTION_MASK_DIRECT_MOVE)
+    rs6000_isa_flags &= ~((OPTION_MASK_P8_VECTOR | OPTION_MASK_CRYPTO)
 		         & ~rs6000_isa_flags_explicit);
 
   if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
@@ -3939,13 +3938,6 @@ rs6000_option_override_internal (bool global_init_p)
       rs6000_isa_flags &= ~OPTION_MASK_FPRND;
     }
 
-  if (TARGET_DIRECT_MOVE && !TARGET_VSX)
-    {
-      if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE)
-	error ("%qs requires %qs", "-mdirect-move", "-mvsx");
-      rs6000_isa_flags &= ~OPTION_MASK_DIRECT_MOVE;
-    }
-
   if (TARGET_P8_VECTOR && !TARGET_ALTIVEC)
     rs6000_isa_flags &= ~OPTION_MASK_P8_VECTOR;
 
@@ -24429,7 +24421,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
 								false, true  },
   { "cmpb",			OPTION_MASK_CMPB,		false, true  },
   { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
-  { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
+  { "direct-move",		0,				false, true  },
   { "dlmzb",			OPTION_MASK_DLMZB,		false, true  },
   { "efficient-unaligned-vsx",	OPTION_MASK_EFFICIENT_UNALIGNED_VSX,
 								false, true  },
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index ce0b14a8d37..647f20de7f2 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -429,19 +429,7 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR6");
   if ((flags & OPTION_MASK_POPCNTD) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR7");
-  /* Note that the OPTION_MASK_DIRECT_MOVE flag is automatically
-     turned on in the following condition:
-     1. TARGET_P8_VECTOR is enabled and OPTION_MASK_DIRECT_MOVE is not
-        explicitly disabled.
-        Hereafter, the OPTION_MASK_DIRECT_MOVE flag is considered to
-        have been turned on explicitly.
-     Note that the OPTION_MASK_DIRECT_MOVE flag is automatically
-     turned off in any of the following conditions:
-     1. TARGET_HARD_FLOAT, TARGET_ALTIVEC, or TARGET_VSX is explicitly
-	disabled and OPTION_MASK_DIRECT_MOVE was not explicitly
-	enabled.
-     2. TARGET_VSX is off.  */
-  if ((flags & OPTION_MASK_DIRECT_MOVE) != 0)
+  if ((flags & OPTION_MASK_P8_VECTOR) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR8");
   if ((flags & OPTION_MASK_MODULO) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
index 28249600318..45dd5a85901 100644
--- a/gcc/config/rs6000/rs6000-cpus.def
+++ b/gcc/config/rs6000/rs6000-cpus.def
@@ -49,7 +49,6 @@
 #define ISA_2_7_MASKS_SERVER	(ISA_2_6_MASKS_SERVER			\
 				 | OPTION_MASK_P8_VECTOR		\
 				 | OPTION_MASK_CRYPTO			\
-				 | OPTION_MASK_DIRECT_MOVE		\
 				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
 				 | OPTION_MASK_QUAD_MEMORY		\
 				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
@@ -90,7 +89,6 @@
 #define OTHER_VSX_VECTOR_MASKS	(OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
 				 | OPTION_MASK_FLOAT128_KEYWORD		\
 				 | OPTION_MASK_P8_VECTOR		\
-				 | OPTION_MASK_DIRECT_MOVE		\
 				 | OPTION_MASK_CRYPTO			\
 				 | OPTION_MASK_P9_VECTOR		\
 				 | OPTION_MASK_FLOAT128_HW		\
@@ -118,7 +116,6 @@
 				 | OPTION_MASK_CMPB			\
 				 | OPTION_MASK_CRYPTO			\
 				 | OPTION_MASK_DFP			\
-				 | OPTION_MASK_DIRECT_MOVE		\
 				 | OPTION_MASK_DLMZB			\
 				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
 				 | OPTION_MASK_FLOAT128_HW		\
diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
index 83197681b66..62131e45bdf 100644
--- a/gcc/config/rs6000/rs6000.opt
+++ b/gcc/config/rs6000/rs6000.opt
@@ -490,8 +490,11 @@ mcrypto
 Target Mask(CRYPTO) Var(rs6000_isa_flags)
 Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
 
+mno-direct-move
+Target Undocumented WarnRemoved
+
 mdirect-move
-Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
+Target Undocumented WarnRemoved
 
 mhtm
 Target Mask(HTM) Var(rs6000_isa_flags)

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Kewen.Lin]

Hi Peter,

on 2024/4/6 06:28, Peter Bergner wrote:
> This is a cleanup patch in preparation to fixing the real bug in PR101865.
> TARGET_DIRECT_MOVE is redundant with TARGET_P8_VECTOR, so alias it to that.
> Also replace all usages of OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR
> and delete the now dead mask.
> 
> This passed bootstrap and retesting on powerpc64le-linux with no regressions.
> Ok for trunk?
> 
> Eventually we'll want to backport this along with the follow-on patch that
> actually fixes PR101865.
> 
> Peter
> 
> 
> gcc/
> 	PR target/101865
> 	* config/rs6000/rs6000.h (TARGET_DIRECT_MOVE): Define.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Replace
> 	OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR.  Delete redundant
> 	OPTION_MASK_DIRECT_MOVE usage.  Delete TARGET_DIRECT_MOVE dead code.
> 	(rs6000_opt_masks): Neuter the "direct-move" option.
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Replace
> 	OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR.  Delete useless
> 	comment.
> 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER): Delete
> 	OPTION_MASK_DIRECT_MOVE.
> 	(OTHER_VSX_VECTOR_MASKS): Likewise.
> 	(POWERPC_MASKS): Likewise.
> 	* config/rs6000/rs6000.opt (mno-direct-move): New.
> 	(mdirect-move): Remove Mask and Var.
> 
> 
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 68bc45d65ba..77d045c9f6e 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -471,6 +471,8 @@ extern int rs6000_vector_align[];
>  #define TARGET_EXTSWSLI	(TARGET_MODULO && TARGET_POWERPC64)
>  #define TARGET_MADDLD	TARGET_MODULO
>  
> +/* TARGET_DIRECT_MOVE is redundant to TARGET_P8_VECTOR, so alias it to that.  */
> +#define TARGET_DIRECT_MOVE	TARGET_P8_VECTOR
>  #define TARGET_XSCVDPSPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
>  #define TARGET_XSCVSPDPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
>  #define TARGET_VADDUQM		(TARGET_P8_VECTOR && TARGET_POWERPC64)
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 6ba9df4f02e..c241371147c 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -3811,7 +3811,7 @@ rs6000_option_override_internal (bool global_init_p)
>       Testing for direct_move matches power8 and later.  */
>    if (!BYTES_BIG_ENDIAN
>        && !(processor_target_table[tune_index].target_enable
> -	   & OPTION_MASK_DIRECT_MOVE))
> +	   & OPTION_MASK_P8_VECTOR))
>      rs6000_isa_flags |= ~rs6000_isa_flags_explicit & OPTION_MASK_STRICT_ALIGN;
>  
>    /* Add some warnings for VSX.  */
> @@ -3853,8 +3853,7 @@ rs6000_option_override_internal (bool global_init_p)
>        && (rs6000_isa_flags_explicit & (OPTION_MASK_SOFT_FLOAT
>  				       | OPTION_MASK_ALTIVEC
>  				       | OPTION_MASK_VSX)) != 0)
> -    rs6000_isa_flags &= ~((OPTION_MASK_P8_VECTOR | OPTION_MASK_CRYPTO
> -			   | OPTION_MASK_DIRECT_MOVE)
> +    rs6000_isa_flags &= ~((OPTION_MASK_P8_VECTOR | OPTION_MASK_CRYPTO)
>  		         & ~rs6000_isa_flags_explicit);
>  
>    if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
> @@ -3939,13 +3938,6 @@ rs6000_option_override_internal (bool global_init_p)
>        rs6000_isa_flags &= ~OPTION_MASK_FPRND;
>      }
>  
> -  if (TARGET_DIRECT_MOVE && !TARGET_VSX)
> -    {
> -      if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE)
> -	error ("%qs requires %qs", "-mdirect-move", "-mvsx");
> -      rs6000_isa_flags &= ~OPTION_MASK_DIRECT_MOVE;
> -    }
> -
>    if (TARGET_P8_VECTOR && !TARGET_ALTIVEC)
>      rs6000_isa_flags &= ~OPTION_MASK_P8_VECTOR;
>  
> @@ -24429,7 +24421,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>  								false, true  },
>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
> -  { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
> +  { "direct-move",		0,				false, true  },
>    { "dlmzb",			OPTION_MASK_DLMZB,		false, true  },
>    { "efficient-unaligned-vsx",	OPTION_MASK_EFFICIENT_UNALIGNED_VSX,
>  								false, true  },
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index ce0b14a8d37..647f20de7f2 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -429,19 +429,7 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR6");
>    if ((flags & OPTION_MASK_POPCNTD) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR7");
> -  /* Note that the OPTION_MASK_DIRECT_MOVE flag is automatically
> -     turned on in the following condition:
> -     1. TARGET_P8_VECTOR is enabled and OPTION_MASK_DIRECT_MOVE is not
> -        explicitly disabled.
> -        Hereafter, the OPTION_MASK_DIRECT_MOVE flag is considered to
> -        have been turned on explicitly.
> -     Note that the OPTION_MASK_DIRECT_MOVE flag is automatically
> -     turned off in any of the following conditions:
> -     1. TARGET_HARD_FLOAT, TARGET_ALTIVEC, or TARGET_VSX is explicitly
> -	disabled and OPTION_MASK_DIRECT_MOVE was not explicitly
> -	enabled.
> -     2. TARGET_VSX is off.  */
> -  if ((flags & OPTION_MASK_DIRECT_MOVE) != 0)
> +  if ((flags & OPTION_MASK_P8_VECTOR) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR8");
>    if ((flags & OPTION_MASK_MODULO) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
> diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
> index 28249600318..45dd5a85901 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -49,7 +49,6 @@
>  #define ISA_2_7_MASKS_SERVER	(ISA_2_6_MASKS_SERVER			\
>  				 | OPTION_MASK_P8_VECTOR		\
>  				 | OPTION_MASK_CRYPTO			\
> -				 | OPTION_MASK_DIRECT_MOVE		\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_QUAD_MEMORY		\
>  				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
> @@ -90,7 +89,6 @@
>  #define OTHER_VSX_VECTOR_MASKS	(OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_FLOAT128_KEYWORD		\
>  				 | OPTION_MASK_P8_VECTOR		\
> -				 | OPTION_MASK_DIRECT_MOVE		\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_P9_VECTOR		\
>  				 | OPTION_MASK_FLOAT128_HW		\
> @@ -118,7 +116,6 @@
>  				 | OPTION_MASK_CMPB			\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_DFP			\
> -				 | OPTION_MASK_DIRECT_MOVE		\
>  				 | OPTION_MASK_DLMZB			\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_FLOAT128_HW		\
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index 83197681b66..62131e45bdf 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -490,8 +490,11 @@ mcrypto
>  Target Mask(CRYPTO) Var(rs6000_isa_flags)
>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
>  
> +mno-direct-move
> +Target Undocumented WarnRemoved
> +
>  mdirect-move
> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Target Undocumented WarnRemoved

When reviewing my previous patch to "neuter option -mpower{8,9}-vector",
Segher mentioned that we don't need to keep such option warning all the
time and can drop it like in a release later as users should be aware of
this information then, I agreed and considering that patch disabling
-m[no-]direct-move was r8-7845-g57f108f5a1e1b2, I think we can just remove
m[no-]direct-move here?  What do you think?

OK for trunk with this tweaked if you agree that, and also ok for active
release branches after burn-in time, thanks!

BR,
Kewen

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Peter Bergner]

On 4/8/24 3:55 AM, Kewen.Lin wrote:
> on 2024/4/6 06:28, Peter Bergner wrote:
>> +mno-direct-move
>> +Target Undocumented WarnRemoved
>> +
>>  mdirect-move
>> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
>> +Target Undocumented WarnRemoved
> 
> When reviewing my previous patch to "neuter option -mpower{8,9}-vector",
> Segher mentioned that we don't need to keep such option warning all the
> time and can drop it like in a release later as users should be aware of
> this information then, I agreed and considering that patch disabling
> -m[no-]direct-move was r8-7845-g57f108f5a1e1b2, I think we can just remove
> m[no-]direct-move here?  What do you think?


I'm fine with that if that is what we want.  So something like the following?

+;; This option existed in the past, but now is always silently ignored.
mdirect-move
-Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
+Target Undocumented Ignore


The above seems to silently ignore both -mdirect-move and -mno-direct-move
which I think is what we want.  That said, it's not what we've done with
other options, but maybe those just need to be changed too?


Peter


;; This option existed in the past, but now is always off.
mno-mfpgpr
Target RejectNegative Undocumented Ignore

mmfpgpr
Target RejectNegative Undocumented WarnRemoved

[snip other examples]

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Kewen.Lin]

Hi Peter,

on 2024/4/8 21:21, Peter Bergner wrote:
> On 4/8/24 3:55 AM, Kewen.Lin wrote:
>> on 2024/4/6 06:28, Peter Bergner wrote:
>>> +mno-direct-move
>>> +Target Undocumented WarnRemoved
>>> +
>>>  mdirect-move
>>> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
>>> +Target Undocumented WarnRemoved
>>
>> When reviewing my previous patch to "neuter option -mpower{8,9}-vector",
>> Segher mentioned that we don't need to keep such option warning all the
>> time and can drop it like in a release later as users should be aware of
>> this information then, I agreed and considering that patch disabling
>> -m[no-]direct-move was r8-7845-g57f108f5a1e1b2, I think we can just remove
>> m[no-]direct-move here?  What do you think?
> 
> 
> I'm fine with that if that is what we want.  So something like the following?
> 
> +;; This option existed in the past, but now is always silently ignored.
> mdirect-move
> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Target Undocumented Ignore

I prefer to remove it completely, that is:

> -mdirect-move
> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved

The reason why you still kept it is to keep a historical record here?

Segher pointed out to me that this kind of option complete removal should be
stage 1 stuff, so let's defer to make it in a separated patch next release
(including some other options like mfpgpr you showed below etc.). :)

For the original patch,

> +mno-direct-move
> +Target Undocumented WarnRemoved

s/WarnRemoved/Ignore/ to match some other existing practice, there is no
warning now if specifying -mno-direct-move and it would be good to keep
the same behavior for users.

OK for trunk and active branches with this tweaked, thanks!

> 
> 
> The above seems to silently ignore both -mdirect-move and -mno-direct-move
> which I think is what we want.  That said, it's not what we've done with
> other options, but maybe those just need to be changed too?

Yes, I think they need to be changed too (next release).

BR,
Kewen

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Peter Bergner]

On 4/8/24 9:37 PM, Kewen.Lin wrote:
> on 2024/4/8 21:21, Peter Bergner wrote:
> I prefer to remove it completely, that is:
> 
>> -mdirect-move
>> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> 
> The reason why you still kept it is to keep a historical record here?

I believe we've never completely removed an option before.  I think the
thought was, if some software package explicitly used the option, then
they shouldn't see an 'unrecognized command-line option' error, but
rather either a warning that the option was removed or just silently
ignore it.  Ie, we don't want to make a package that used to build with
an old compiler now break its build because the option doesn't exist
anymore.



> Segher pointed out to me that this kind of option complete removal should be
> stage 1 stuff, so let's defer to make it in a separated patch next release
> (including some other options like mfpgpr you showed below etc.). :)

If we're going to completely remove it, then for sure, it's a stage1 thing.
I'd like to hear Segher's thoughts on whether we should completely remove
it or just silently ignore it.



> For the original patch,
> 
>> +mno-direct-move
>> +Target Undocumented WarnRemoved
> 
> s/WarnRemoved/Ignore/ to match some other existing practice, there is no
> warning now if specifying -mno-direct-move and it would be good to keep
> the same behavior for users.

If we want to silently ignore -mdirect-move and -mno-direct-move, then we
just need to do:

mdirect-move
-Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
+Target Undocumented Ignore

There's no need to mention -mno-direct-move at all then.  It was only in the
case I thought we wanted to warn against it's use that I added -mno-direct-move.



>> That said, it's not what we've done with
>> other options, but maybe those just need to be changed too?
> 
> Yes, I think they need to be changed too (next release).

If that's the consensus with Segher, sure, we can plan on that in stage1.

Peter

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Kewen.Lin]

on 2024/4/9 11:20, Peter Bergner wrote:
> On 4/8/24 9:37 PM, Kewen.Lin wrote:
>> on 2024/4/8 21:21, Peter Bergner wrote:
>> I prefer to remove it completely, that is:
>>
>>> -mdirect-move
>>> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
>>
>> The reason why you still kept it is to keep a historical record here?
> 
> I believe we've never completely removed an option before.  I think the

By checking the history, we did remove some options for SPE, paired single,
xilinx-fpu etc., which can be taken as gone with feature removal, but also
-maltivec={le,be} and -misel={yes,no}.

> thought was, if some software package explicitly used the option, then
> they shouldn't see an 'unrecognized command-line option' error, but
> rather either a warning that the option was removed or just silently
> ignore it.  Ie, we don't want to make a package that used to build with
> an old compiler now break its build because the option doesn't exist
> anymore.

I understand, but an argument is that no errors (even no warnings) can imply
some option still takes effect and cause some misunderstanding easily.  For
the release in which we remove the support of an option, we can still mark
it as WarnRemoved, but after a release or so, users should be aware of this
change and modify their build scripts if need, it's better to emit errors
for them to avoid the false appearance that it's still supported.

> 
>> Segher pointed out to me that this kind of option complete removal should be
>> stage 1 stuff, so let's defer to make it in a separated patch next release
>> (including some other options like mfpgpr you showed below etc.). :)
> 
> If we're going to completely remove it, then for sure, it's a stage1 thing.
> I'd like to hear Segher's thoughts on whether we should completely remove
> it or just silently ignore it.
> 
> 
> 
>> For the original patch,
>>
>>> +mno-direct-move
>>> +Target Undocumented WarnRemoved
>>
>> s/WarnRemoved/Ignore/ to match some other existing practice, there is no
>> warning now if specifying -mno-direct-move and it would be good to keep
>> the same behavior for users.
> 
> If we want to silently ignore -mdirect-move and -mno-direct-move, then we
> just need to do:
> 
> mdirect-move
> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Target Undocumented Ignore
> 

Since removing it completely is a stage1 thing, I prefer to keep mdirect-move
and -mno-direct-move handlings as before, WarnRemoved and Ignore separately.

> There's no need to mention -mno-direct-move at all then.  It was only in the
> case I thought we wanted to warn against it's use that I added -mno-direct-move.
> 
> 

Not to mention it is fine too, just keep the handlings and defer it to stage 1. :)

BR,
Kewen

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Peter Bergner]

On 4/9/24 12:37 AM, Kewen.Lin wrote:
> Since removing it completely is a stage1 thing, I prefer to keep mdirect-move
> and -mno-direct-move handlings as before, WarnRemoved and Ignore separately.

Ok, current trunk ignores -mno-direct-move and warns on -mdirect-move, so to
keep the same behavior for GCC 14 (before removing in stage1), we want just:

mdirect-move
-Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
+Target Undocumented WarnRemoved

....which is what I'll commit and push after one last round of testing.
Thanks.

Peter

Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]

[Peter Bergner]

On 4/9/24 3:19 PM, Peter Bergner wrote:
> Ok, current trunk ignores -mno-direct-move and warns on -mdirect-move, so to
> keep the same behavior for GCC 14 (before removing in stage1), we want just:
> 
> mdirect-move
> -Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Target Undocumented WarnRemoved
> 
> ....which is what I'll commit and push after one last round of testing.

Testing was clean as expected, so I pushed the commit.  Thanks.

Peter