[rs6000] Eliminate TARGET_CTZ,TARGET_FCTIDZ,FCTIWUZ defines

  [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.
  

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
  

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
  

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