Re: [PATCH v3 3/3] RISC-V: Add support for XCVbi extension in CV32E40P

[Kito Cheng <kito.cheng@gmail.com>]

On Tue, Nov 28, 2023 at 9:17 PM Mary Bennett <mary.bennett@embecosm.com> wrote:
>
> Spec: github.com/openhwgroup/core-v-sw/blob/master/specifications/corev-builtin-spec.md
>
> Contributors:
>   Mary Bennett <mary.bennett@embecosm.com>
>   Nandni Jamnadas <nandni.jamnadas@embecosm.com>
>   Pietra Ferreira <pietra.ferreira@embecosm.com>
>   Charlie Keaney
>   Jessica Mills
>   Craig Blackmore <craig.blackmore@embecosm.com>
>   Simon Cook <simon.cook@embecosm.com>
>   Jeremy Bennett <jeremy.bennett@embecosm.com>
>   Helene Chelin <helene.chelin@embecosm.com>
>
> gcc/ChangeLog:
>         * common/config/riscv/riscv-common.cc: Create XCVbi extension
>           support.
>         * config/riscv/riscv.opt: Likewise.
>         * config/riscv/corev.md: Implement cv_branch<mode> pattern
>           for cv.beqimm and cv.bneimm.
>         * config/riscv/riscv.md: Add CORE-V branch immediate to RISC-V
>           branch instruction pattern.
>         * config/riscv/constraints.md: Implement constraints
>           cv_bi_s5 - signed 5-bit immediate.
>         * config/riscv/predicates.md: Implement predicate
>           const_int5s_operand - signed 5 bit immediate.
>         * doc/sourcebuild.texi: Add XCVbi documentation.
>
> gcc/testsuite/ChangeLog:
>         * gcc.target/riscv/cv-bi-beqimm-compile-1.c: New test.
>         * gcc.target/riscv/cv-bi-beqimm-compile-2.c: New test.
>         * gcc.target/riscv/cv-bi-bneimm-compile-1.c: New test.
>         * gcc.target/riscv/cv-bi-bneimm-compile-2.c: New test.
>         * lib/target-supports.exp: Add proc for XCVbi.
> ---
>  gcc/common/config/riscv/riscv-common.cc       |  2 +
>  gcc/config/riscv/constraints.md               |  6 +++
>  gcc/config/riscv/corev.md                     | 14 ++++++
>  gcc/config/riscv/predicates.md                |  4 ++
>  gcc/config/riscv/riscv.md                     |  4 ++
>  gcc/config/riscv/riscv.opt                    |  2 +
>  gcc/doc/sourcebuild.texi                      |  3 ++
>  .../gcc.target/riscv/cv-bi-beqimm-compile-1.c | 17 +++++++
>  .../gcc.target/riscv/cv-bi-beqimm-compile-2.c | 48 +++++++++++++++++++
>  .../gcc.target/riscv/cv-bi-bneimm-compile-1.c | 17 +++++++
>  .../gcc.target/riscv/cv-bi-bneimm-compile-2.c | 48 +++++++++++++++++++
>  gcc/testsuite/lib/target-supports.exp         | 13 +++++
>  12 files changed, 178 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/cv-bi-beqimm-compile-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/cv-bi-beqimm-compile-2.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/cv-bi-bneimm-compile-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/cv-bi-bneimm-compile-2.c
>
> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
> index c8c0d0a2252..125f8fb71f7 100644
> --- a/gcc/common/config/riscv/riscv-common.cc
> +++ b/gcc/common/config/riscv/riscv-common.cc
> @@ -313,6 +313,7 @@ static const struct riscv_ext_version riscv_ext_version_table[] =
>    {"xcvmac", ISA_SPEC_CLASS_NONE, 1, 0},
>    {"xcvalu", ISA_SPEC_CLASS_NONE, 1, 0},
>    {"xcvelw", ISA_SPEC_CLASS_NONE, 1, 0},
> +  {"xcvbi", ISA_SPEC_CLASS_NONE, 1, 0},
>
>    {"xtheadba", ISA_SPEC_CLASS_NONE, 1, 0},
>    {"xtheadbb", ISA_SPEC_CLASS_NONE, 1, 0},
> @@ -1678,6 +1679,7 @@ static const riscv_ext_flag_table_t riscv_ext_flag_table[] =
>    {"xcvmac",        &gcc_options::x_riscv_xcv_subext, MASK_XCVMAC},
>    {"xcvalu",        &gcc_options::x_riscv_xcv_subext, MASK_XCVALU},
>    {"xcvelw",        &gcc_options::x_riscv_xcv_subext, MASK_XCVELW},
> +  {"xcvbi",         &gcc_options::x_riscv_xcv_subext, MASK_XCVBI},
>
>    {"xtheadba",      &gcc_options::x_riscv_xthead_subext, MASK_XTHEADBA},
>    {"xtheadbb",      &gcc_options::x_riscv_xthead_subext, MASK_XTHEADBB},
> diff --git a/gcc/config/riscv/constraints.md b/gcc/config/riscv/constraints.md
> index 2711efe68c5..718b4bd77df 100644
> --- a/gcc/config/riscv/constraints.md
> +++ b/gcc/config/riscv/constraints.md
> @@ -247,3 +247,9 @@
>    (and (match_code "const_int")
>         (and (match_test "IN_RANGE (ival, 0, 1073741823)")
>              (match_test "exact_log2 (ival + 1) != -1"))))
> +
> +(define_constraint "CV_bi_sign5"
> +  "@internal
> +   A 5-bit signed immediate for CORE-V Immediate Branch."
> +  (and (match_code "const_int")
> +       (match_test "IN_RANGE (ival, -16, 15)")))
> diff --git a/gcc/config/riscv/corev.md b/gcc/config/riscv/corev.md
> index 92bf0b5d6a6..f6a1f916d7e 100644
> --- a/gcc/config/riscv/corev.md
> +++ b/gcc/config/riscv/corev.md
> @@ -706,3 +706,17 @@
>
>    [(set_attr "type" "load")
>    (set_attr "mode" "SI")])
> +
> +;; XCVBI Builtins

It's not builtin I think? maybe just "XCVBI Instructions"

> +(define_insn "cv_branch<mode>"

"*cv_branch<mode>"

> +  [(set (pc)
> +       (if_then_else
> +        (match_operator 1 "equality_operator"
> +                        [(match_operand:X 2 "register_operand" "r")
> +                         (match_operand:X 3 "const_int5s_operand" "CV_bi_sign5")])
> +        (label_ref (match_operand 0 "" ""))
> +        (pc)))]
> +  "TARGET_XCVBI"
> +  "cv.b%C1imm\t%2,%3,%0"

And then duplicate content of "*branch<mode>" here.

> +  [(set_attr "type" "branch")
> +   (set_attr "mode" "none")])
> diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> index ff213e5f8a3..dfe0db02ac1 100644
> --- a/gcc/config/riscv/predicates.md
> +++ b/gcc/config/riscv/predicates.md
> @@ -406,6 +406,10 @@
>    (ior (match_operand 0 "register_operand")
>         (match_code "const_int")))
>
> +(define_predicate "const_int5s_operand"
> +  (and (match_code "const_int")
> +       (match_test "IN_RANGE (INTVAL (op), -16, 15)")))
> +
>  ;; Predicates for the V extension.
>  (define_special_predicate "vector_length_operand"
>    (ior (match_operand 0 "pmode_register_operand")
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 935eeb7fd8e..467cd09d8b0 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -2647,6 +2647,10 @@
>          (pc)))]
>    ""

I would prefer to put (!TARGET_XCVBI || !equality_operator
(operands[1], <MODE>mode)) here,
The intention of that is dispatch == and != to cv_branch if it is enabled.

>  {
> +  if (TARGET_XCVBI && const_int5s_operand (operands[3], SImode)
> +       && (GET_CODE (operands[1]) == EQ || GET_CODE (operands[1]) == NE))
> +    return "cv.b%C1imm\t%2,%3,%0";

then this is not necessary, just use cv_branch,
and I guess this can't not handle long branches well, you may need
something like that:

    if (get_attr_length (insn) == 12)
      return "cv.b%N1\t%2,%z3,1f; jump\t%l0,ra; 1:";


> +
>    if (get_attr_length (insn) == 12)
>      return "b%N1\t%2,%z3,1f; jump\t%l0,ra; 1:";