[PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

[PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

[will schmidt]

[PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

Hi,
  This is the first of a batch of changes that eliminate a number
of define TARGET_foo entries we have collected over time.

TARGET_CTZ is defined as TARGET_MODULO, and has a low number
of uses.  References to TARGET_CTZ should be safe to replace
with TARGET_MODULO throughout.

TARGET_FCTIDZ is entirely unused, and safe to remove.

TARGET_FCTIWUZ has a low number of uses, and can be directly
replaced with TARGET_POPCNTD.

This eliminates three defines.

There should be no codegen changes, and this has regtested OK.
OK for trunk?
Thanks,
    
    gcc/
            * config/rs6000/rs6000.h (TARGET_CTZ): Replace with
            TARGET_MODULO.
            (TARGET_FCTIDZ): Remove.
            (TARGET_FCTIWUZ): Replace with TARGET_POPCNTD.
            * config/rs6000/rs6000.cc (TARGET_CTZ): Replace with TARGET_MODULO.
            * config/rs6000/rs6000.md (ctz<mode>2): Replace TARGET_CTZ
            with TARGET_MODULO.
            (ctz<mode>2_hw): Same.
            (fixuns_trunc<mode>si2): Replace TARGET_FCTIWUZ
            with TARGET_POPCNTD.
            (fixuns_trunc<mode>si2_stfiwx): Same.
            (fctiwz_<mode>): Same.

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index fcca062a8709..eea427b1ca51 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -21998,11 +21998,11 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
       if (!TARGET_MODULO && (code == MOD || code == UMOD))
 	*total += COSTS_N_INSNS (2);
       return false;
 
     case CTZ:
-      *total = COSTS_N_INSNS (TARGET_CTZ ? 1 : 4);
+      *total = COSTS_N_INSNS (TARGET_MODULO ? 1 : 4);
       return false;
 
     case FFS:
       *total = COSTS_N_INSNS (4);
       return false;
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index eb7b21584970..ee887efd1122 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -456,20 +456,17 @@ extern int rs6000_vector_align[];
 			 || TARGET_PPC_GPOPT	/* 970/power4 */	\
 			 || TARGET_POPCNTB	/* ISA 2.02 */		\
 			 || TARGET_CMPB		/* ISA 2.05 */		\
 			 || TARGET_POPCNTD)	/* ISA 2.06 */
 
-#define TARGET_FCTIDZ	TARGET_FCFID
 #define TARGET_STFIWX	TARGET_PPC_GFXOPT
 #define TARGET_LFIWAX	TARGET_CMPB
 #define TARGET_LFIWZX	TARGET_POPCNTD
 #define TARGET_FCFIDS	TARGET_POPCNTD
 #define TARGET_FCFIDU	TARGET_POPCNTD
 #define TARGET_FCFIDUS	TARGET_POPCNTD
 #define TARGET_FCTIDUZ	TARGET_POPCNTD
-#define TARGET_FCTIWUZ	TARGET_POPCNTD
-#define TARGET_CTZ	TARGET_MODULO
 #define TARGET_EXTSWSLI	(TARGET_MODULO && TARGET_POWERPC64)
 #define TARGET_MADDLD	TARGET_MODULO
 
 #define TARGET_XSCVDPSPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
 #define TARGET_XSCVSPDPN	(TARGET_DIRECT_MOVE || TARGET_P8_VECTOR)
@@ -1751,11 +1748,11 @@ typedef struct rs6000_args
 
 /* The CTZ patterns that are implemented in terms of CLZ return -1 for input of
    zero.  The hardware instructions added in Power9 and the sequences using
    popcount return 32 or 64.  */
 #define CTZ_DEFINED_VALUE_AT_ZERO(MODE, VALUE)				\
-  (TARGET_CTZ || TARGET_POPCNTD						\
+  (TARGET_MODULO || TARGET_POPCNTD						\
    ? ((VALUE) = GET_MODE_BITSIZE (MODE), 2)				\
    : ((VALUE) = -1, 2))
 
 /* Specify the machine mode that pointers have.
    After generation of rtl, the compiler makes no further distinction
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index ad5a4cf2ef83..619a87374734 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -2414,11 +2414,11 @@ (define_insn "clz<mode>2"
 (define_expand "ctz<mode>2"
    [(set (match_operand:GPR 0 "gpc_reg_operand")
 	 (ctz:GPR (match_operand:GPR 1 "gpc_reg_operand")))]
   ""
 {
-  if (TARGET_CTZ)
+  if (TARGET_MODULO)
     {
       emit_insn (gen_ctz<mode>2_hw (operands[0], operands[1]));
       DONE;
     }
 
@@ -2445,11 +2445,11 @@ (define_expand "ctz<mode>2"
 })
 
 (define_insn "ctz<mode>2_hw"
   [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
 	(ctz:GPR (match_operand:GPR 1 "gpc_reg_operand" "r")))]
-  "TARGET_CTZ"
+  "TARGET_MODULO"
   "cnttz<wd> %0,%1"
   [(set_attr "type" "cntlz")])
 
 (define_expand "ffs<mode>2"
   [(set (match_operand:GPR 0 "gpc_reg_operand")
@@ -6326,11 +6326,11 @@ (define_insn_and_split "*fix<uns>_trunc<SFDF:mode><QHSI:mode>2_mem"
 })
 
 (define_expand "fixuns_trunc<mode>si2"
   [(set (match_operand:SI 0 "gpc_reg_operand")
 	(unsigned_fix:SI (match_operand:SFDF 1 "gpc_reg_operand")))]
-  "TARGET_HARD_FLOAT && TARGET_FCTIWUZ && TARGET_STFIWX"
+  "TARGET_HARD_FLOAT && TARGET_POPCNTD && TARGET_STFIWX"
 {
   if (!TARGET_P8_VECTOR)
     {
       emit_insn (gen_fixuns_trunc<mode>si2_stfiwx (operands[0], operands[1]));
       DONE;
@@ -6339,11 +6339,11 @@ (define_expand "fixuns_trunc<mode>si2"
 
 (define_insn_and_split "fixuns_trunc<mode>si2_stfiwx"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=rm")
 	(unsigned_fix:SI (match_operand:SFDF 1 "gpc_reg_operand" "d")))
    (clobber (match_scratch:DI 2 "=d"))]
-  "TARGET_HARD_FLOAT && TARGET_FCTIWUZ
+  "TARGET_HARD_FLOAT && TARGET_POPCNTD
    && TARGET_STFIWX && can_create_pseudo_p ()
    && !TARGET_P8_VECTOR"
   "#"
   "&& 1"
   [(pc)]
@@ -6542,11 +6542,11 @@ (define_insn "fctiwz_<mode>"
 (define_insn "fctiwuz_<mode>"
   [(set (match_operand:DI 0 "gpc_reg_operand" "=d,wa")
 	(unspec:DI [(unsigned_fix:SI
 		     (match_operand:SFDF 1 "gpc_reg_operand" "d,wa"))]
 		   UNSPEC_FCTIWUZ))]
-  "TARGET_HARD_FLOAT && TARGET_FCTIWUZ"
+  "TARGET_HARD_FLOAT && TARGET_POPCNTD"
   "@
    fctiwuz %0,%1
    xscvdpuxws %x0,%x1"
   [(set_attr "type" "fp")])
 

Re: [PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

[Segher Boessenkool]

Hi!

On Mon, Sep 19, 2022 at 06:19:15PM -0500, will schmidt wrote:
>   This is the first of a batch of changes that eliminate a number
> of define TARGET_foo entries we have collected over time.

Good good :-)

> TARGET_CTZ is defined as TARGET_MODULO, and has a low number
> of uses.  References to TARGET_CTZ should be safe to replace
> with TARGET_MODULO throughout.

No, please don't.  This has nothing to with "modulo".  If you want to
say this is just whether we have ISA 3.0 or p9, make a new target macro
for *that* and use that everywhere.

This is a general issue, that will make the code much more sane if you
can fix it!

> TARGET_FCTIDZ is entirely unused, and safe to remove.

Please make separate patches for separate issues.  This makes it much
easier to review, and MUCH easier for all other ways we need to handle
it (backports, reverts, everything else).  With Git it is *easier* to
keep separate patches separate than it is to lump it all together.  So,
the trick is to keep things in separate commits during development
already (and you will find more benefits doing that, too!)

TARGET_FCTIDZ was never used, it always used TARGET_FCFID directly.

The original PEM mistakenly said this insn is "64-bit only".  This was
fixed in ISA 2.01 .

> TARGET_FCTIWUZ has a low number of uses, and can be directly
> replaced with TARGET_POPCNTD.

It is a p7 (ISA 2.06) insn.  Please make a TARGET_P7 or such?

In the current situation target macros like TARGET_POPCNTD are abused to
mean either "can we use the popcntd insn", or to mean "can we use insn
new on p7".  Or sometimes something in between, or something in this
general neighbourhood.  It is never clear which is meant, which makes it
very hard to untangle this.  But thanks for trying!  :-)

(Don't let me dicsourage you btw, most is pretty straightforward).

>             * config/rs6000/rs6000.h (TARGET_CTZ): Replace with
>             TARGET_MODULO.

Changelogs are indented with tabs, and this fits on one line.

So, please make TARGET_P7 and such, and OPTION_MASKs for those in
rs6000-cpus.def?


Segher

Re: [PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

[will schmidt]

On Tue, 2022-09-20 at 16:14 -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Sep 19, 2022 at 06:19:15PM -0500, will schmidt wrote:
> >   This is the first of a batch of changes that eliminate a number
> > of define TARGET_foo entries we have collected over time.
> 
> Good good :-)
> 
> > TARGET_CTZ is defined as TARGET_MODULO, and has a low number
> > of uses.  References to TARGET_CTZ should be safe to replace
> > with TARGET_MODULO throughout.
> 
> No, please don't.  This has nothing to with "modulo".  If you want to
> say this is just whether we have ISA 3.0 or p9, make a new target
> macro
> for *that* and use that everywhere.
> 
> This is a general issue, that will make the code much more sane if
> you
> can fix it!

> 
> > TARGET_FCTIDZ is entirely unused, and safe to remove.
> 
> Please make separate patches for separate issues.  This makes it much
> easier to review, and MUCH easier for all other ways we need to
> handle
> it (backports, reverts, everything else).  With Git it is *easier* to
> keep separate patches separate than it is to lump it all
> together.  So,
> the trick is to keep things in separate commits during development
> already (and you will find more benefits doing that, too!)

Yup, I actually developed these three (plus a bunch more) separately,
but combined the first three for posting.   I'll split them back out
and repost after a bit. 

> 
> TARGET_FCTIDZ was never used, it always used TARGET_FCFID directly.
> 
> The original PEM mistakenly said this insn is "64-bit only".  This
> was
> fixed in ISA 2.01 .
> 
> > TARGET_FCTIWUZ has a low number of uses, and can be directly
> > replaced with TARGET_POPCNTD.
> 
> It is a p7 (ISA 2.06) insn.  Please make a TARGET_P7 or such?


Yes.  I do have a change later in the (unposted) series to replace
POPCNTD with POWER7, at a glance thats #17 down the line. In review I
agree with your comment that the in-between changes aren't the best
choices. I'll see about skipping the in-between values and going
straight for POPCNTD->POWER7.

I am looking at the TARGET_POWER10 notation as the target style, versus
TARGET_P7, but I can go that direction if we think that would be
preferred.   Maybe it is since this is a retro-fix versus new. :-)


> 
> In the current situation target macros like TARGET_POPCNTD are abused
> to
> mean either "can we use the popcntd insn", or to mean "can we use
> insn
> new on p7".  Or sometimes something in between, or something in this
> general neighbourhood.  It is never clear which is meant, which makes
> it
> very hard to untangle this.  But thanks for trying!  :-)
>
> (Don't let me dicsourage you btw, most is pretty straightforward).

Absolutely..   I do have this mostly covered locally, I just need to
refine a few parts.  :-)

> 
> 
> >             * config/rs6000/rs6000.h (TARGET_CTZ): Replace with
> >             TARGET_MODULO.
> 
> Changelogs are indented with tabs, and this fits on one line.
> 
> So, please make TARGET_P7 and such, and OPTION_MASKs for those in
> rs6000-cpus.def?

willdo, 
thanks
-Will


> 
> 
> Segher

Re: [PATCH, rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

[Segher Boessenkool]

On Tue, Sep 20, 2022 at 05:01:53PM -0500, will schmidt wrote:
> On Tue, 2022-09-20 at 16:14 -0500, Segher Boessenkool wrote:
> > > TARGET_FCTIWUZ has a low number of uses, and can be directly
> > > replaced with TARGET_POPCNTD.
> > 
> > It is a p7 (ISA 2.06) insn.  Please make a TARGET_P7 or such?
> 
> Yes.  I do have a change later in the (unposted) series to replace
> POPCNTD with POWER7, at a glance thats #17 down the line. In review I
> agree with your comment that the in-between changes aren't the best
> choices. I'll see about skipping the in-between values and going
> straight for POPCNTD->POWER7.

First make new TARGET_Px and OPTION_MASK_Px for all "x" you want,
and do nothing else than enabling it in the respective CPUs in
rs6000-cpus.def .  This can be just one patch of course, it is
a) bloody simple and b) all is the same.  Have that as the very first
patch.  After that most things will be simple and obvious.  But please
do keep most later things split out, it is much easier to review.

> I am looking at the TARGET_POWER10 notation as the target style, versus
> TARGET_P7, but I can go that direction if we think that would be
> preferred.   Maybe it is since this is a retro-fix versus new. :-)

I think TARGET_P7 is a nicely shorter name.  It adds up :-)  The
existing TARGET_P10_SOMETHING do not write it out either btw (and same
for P9 and P8).

But this is not very important of course.  It helps to pick good names
from the get go of course, much less work than fixing things later.

> > (Don't let me dicsourage you btw, most is pretty straightforward).
> 
> Absolutely..   I do have this mostly covered locally, I just need to
> refine a few parts.  :-)

Looking forward to it!


Segher