[Bug target/108248] New: Some insns in the risc-v backend do not have mappings to functional units

[Bug target/108248] New: Some insns in the risc-v backend do not have mappings to functional units

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

            Bug ID: 108248
           Summary: Some insns in the risc-v backend do not have mappings
                    to functional units
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: law at gcc dot gnu.org
                CC: rzinsly at ventanamicro dot com
  Target Milestone: ---
            Target: risc-v

If you review scheduling dumps for risc-v you will see various insns with no
reservations.  For example:

;;       11--> b  3: i  58 r184=smax(r154,r183)                    :nothing
;;        3--> b  3: i  55 r182=r180<<0x3+r160                     :nothing
;;        8--> b  0: i  16 r167=r193<<0x3+r193                     :nothing
;;       13--> b  0: i  19 r155=r167<<0x3+r188                     :nothing
;;        0--> b  0: i  26 r172=r164<<0x3+r164                     :nothing
;;        2--> b  0: i  23 r144=r161<<0x3+r159                     :nothing

It's worth noting these all seem to be coming from the bitmanip extensions. 
However, there well could be others.

It would be advisable to ensure that all insns have appropriate mappings to
functional units.

This patch from Jivan would help, but I think a thorough review to ensure
everything has a mapping would be advisable:
diff --git a/gcc/config/riscv/generic.md b/gcc/config/riscv/generic.md
index 1a209dcb997..296c4e24923 100644
--- a/gcc/config/riscv/generic.md
+++ b/gcc/config/riscv/generic.md
@@ -88,3 +88,8 @@
   (and (eq_attr "tune" "generic")
        (eq_attr "type" "fsqrt"))
   "fdivsqrt*25")
+
+(define_insn_reservation "generic_smax" 1
+  (and (eq_attr "tune" "generic")
+       (eq_attr "type" "bitmanip"))
+  "alu")

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[andrew at sifive dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

Andrew Waterman <andrew at sifive dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew at sifive dot com

--- Comment #1 from Andrew Waterman <andrew at sifive dot com> ---
What will the scheduler do when tuning for a pipeline model that doesn't
explicitly handle type=bitmanip?

Additional work is needed to make the sifive-7 model do the right thing, but
it's not of especially high priority if the default behavior isn't unduly
weird.

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #2 from Jeffrey A. Law <law at gcc dot gnu.org> ---
It can certainly get "unduly weird".  Basically such instructions get put at
the end of the ready queue as soon as its input dependencies are satisfied.  If
there's only a few such instructions, then the resultant schedule will be
reasonably sane.  But if all or most don't have a mapping to reservations, then
the result can be totally insane and worse than not scheduling at all.

I saw a case internally where a bunch of loads were ready to issue at the start
of a loop.  So those all go onto the ready list and get pulled off one by one
in the same order in which they were added to the ready list. As the loads
issue, dependent instructions get put on the tail of the ready list and would
issue after *all* the originally ready loads issued.  The result was the loop
had its loads bunched up at the top and the multi-cycle computation
instructions at the bottom.  We'd have been better off not scheduling it at all
as the original order interleaved the loads and computation.  This all occurred
because we didn't have any reservations defined for our chip -- so everything
was "nothing".

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[andrew at sifive dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #3 from Andrew Waterman <andrew at sifive dot com> ---
Yikes.  Thanks for the explanation, Jeff.

(cc Kito Cheng: at some point, we should revisit the pipeline modeling of Zb*
instructions for sifive-7.  The short version is that all Zb* instructions can
execute on the B pipe.  The A pipe supports only a subset: rev8, bext[i],
addu.w, slliu.w, sext.*, zext.*, orn, xnor, andn.  So, to do an accurate job,
we'll need finer granularity than simply describing them all as "bitmanip".)

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #4 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Yea, thinking about our uarch, we're going to need finer control as well. 
There's a subset that ought to execute in any ALU, but there's another subset
that are bound to a specific ALU and are potentially multi-cycle latency.

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #5 from Jeffrey A. Law <law at gcc dot gnu.org> ---
So a datapoint in this effort.

For the Veyron V1, all the bitmanip instructions except clmul and cpop are
single cycle and can be handled by any of the 4 standard ALUs.

clmul, cpop are 4c and use the shared multi-cycle ALU.


Obviously we may need to break things down further for other uarchs.  But
that's start.

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #5)
> So a datapoint in this effort.
> 
> For the Veyron V1, all the bitmanip instructions except clmul and cpop are
> single cycle and can be handled by any of the 4 standard ALUs.
> 
> clmul, cpop are 4c and use the shared multi-cycle ALU.

This is what I mostly expect for a standard core even. clmul and cpop would be
very similar the mul instruction even.
I hearing our core will be very similar (the core is still in design) but I
will definitely be posting patches to make sure our core's cost model is
correct and all once it is finished.

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:07e2576d6f344acab338deeb051845c90c1cf6a3

commit r14-116-g07e2576d6f344acab338deeb051845c90c1cf6a3
Author: Raphael Zinsly <rzinsly@ventanamicro.com>
Date:   Thu Apr 20 08:48:08 2023 -0600

    [PR target/108248] [RISC-V] Break down some bitmanip insn types

    This is primarily Raphael's work.  All I did was adjust it to apply to the
    trunk and add the new types to generic.md's scheduling model.

    The basic idea here is to make sure we have the ability to schedule the
    bitmanip instructions with a finer degree of control.  Some of the bitmanip
    instructions are likely to have differing scheduler characteristics across
    different implementations.

    So rather than assign these instructions a generic "bitmanip" type, this
    patch assigns them a type based on their RTL code by using the
<bitmanip_insn>
    iterator for the type.  Naturally we have to add a few new types.  It
affects
    clz, ctz, cpop, min, max.

    We didn't do this for things like shNadd, single bit manipulation, etc. We
    certainly could if the needs presents itself.

    I threw all the new types into the generic_alu bucket in the generic
    scheduling model.  Seems as good a place as any. Someone who knows the
    sifive uarch should probably add these types (and bitmanip) to the sifive
    scheduling model.

    We also noticed that the recently added orc.b didn't have a type at all.
    So we added it as a generic bitmanip type.

    This has been bootstrapped in a gcc-12 base and I've built and run the
    testsuite without regressions on the trunk.

    Given it was primarily Raphael's work I could probably approve & commit it.
    But I'd like to give the other RISC-V folks a chance to chime in.

            PR target/108248
    gcc/
            * config/riscv/bitmanip.md (clz, ctz, pcnt, min, max patterns): Use
            <bitmanip_insn> as the type to allow for fine grained control of
            scheduling these insns.
            * config/riscv/generic.md (generic_alu): Add bitmanip, clz, ctz,
pcnt,
            min, max.
            * config/riscv/riscv.md (type attribute): Add types for clz, ctz,
            pcnt, signed and unsigned min/max.

[Bug target/108248] Some insns in the risc-v backend do not have mappings to functional units

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108248

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Fixed on the trunk.