Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]
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
Re: [PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]
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]
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]
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]
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]
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]
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_MADDLDTARGET_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
[PATCH] rs6000: Replace OPTION_MASK_DIRECT_MOVE with OPTION_MASK_P8_VECTOR [PR101865]
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,
