Re: [committed v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model

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

On Wed, Nov 3, 2021 at 12:07 AM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> Fix a build regression from commit 04a9b554ba1a ("RISC-V: Cost model
> for zba extension."):
>
> .../gcc/config/riscv/riscv.c: In function 'bool riscv_rtx_costs(rtx, machine_mode, int, int, int*, bool)':
> .../gcc/config/riscv/riscv.c:2018:11: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror]
>  2018 |           && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3))
>       |           ^~
> .../gcc/config/riscv/riscv.c:2047:17: error: unused variable 'ashift_lhs' [-Werror=unused-variable]
>  2047 |             rtx ashift_lhs = XEXP (and_lhs, 0);
>       |                 ^~~~~~~~~~
>
>
> by correcting a CONST_INT_P check referring the wrong operand and
> getting rid of the unused variable.
>
>         gcc/
>         * config/riscv/riscv.c (riscv_rtx_costs): Correct a CONST_INT_P
>         check and remove an unused local variable with shNadd/shNadd.uw
>         pattern handling.
> ---
> Hi Kito,
>
> > I think that's my mistake...it should fix following check rather than
> > remove the REG_P like that:
> >
> > @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mode, int
> > outer_code, int opno ATTRIBUTE_UN
> >              (TARGET_64BIT && (mode == DImode)))
> >          && (GET_CODE (XEXP (x, 0)) == ASHIFT)
> >          && REG_P (XEXP (XEXP (x, 0), 0))
> > -         && CONST_INT_P (XEXP (XEXP (x, 0), 0))
> > -         && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3))
> > +         && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> > +         && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3))
> >        {
> >          *total = COSTS_N_INSNS (1);
> >          return true;
> >
> >
> > shNadd pattern:
> >
> > (define_insn "*shNadd"
> >  [(set (match_operand:X 0 "register_operand" "=r")
> >        (plus:X (ashift:X (match_operand:X 1 "register_operand" "r")
> >                          # What I want to check is here, it should be
> > XEXP (XEXP (x, 0), 1)
> >                          (match_operand:QI 2 "immediate_operand" "I"))
> >                (match_operand:X 3 "register_operand" "r")))]
>
>  Right, I should have cross-checked with the machine description.
>
>  Also are we missing explicit test coverage here?  Or is it supposed to
> be covered by the generic tests here or there already (I'm not familiar
> with the details of the ISA extension to tell offhand), as long as the
> extension has been enabled for the target tested, and it is just that
> the problem has slipped through somehow?

Cost model is not easy to test (at least to me :p), I usually verify
that by check the dump of combine pass to make sure the cost is right.

> > Otherwise LGTM, feel free to commit once you address this issue.
>
>  Rebuilt for verification and committed as shown.  Thank you for your
> review.

Thanks!

>
>   Maciej
> ---
>  gcc/config/riscv/riscv.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> gcc-riscv-rtx-costs-zba-shnadd.diff
> Index: gcc/gcc/config/riscv/riscv.c
> ===================================================================
> --- gcc.orig/gcc/config/riscv/riscv.c
> +++ gcc/gcc/config/riscv/riscv.c
> @@ -2014,8 +2014,8 @@ riscv_rtx_costs (rtx x, machine_mode mod
>               (TARGET_64BIT && (mode == DImode)))
>           && (GET_CODE (XEXP (x, 0)) == ASHIFT)
>           && REG_P (XEXP (XEXP (x, 0), 0))
> -         && CONST_INT_P (XEXP (XEXP (x, 0), 0))
> -         && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 0)), 1, 3))
> +         && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> +         && IN_RANGE (INTVAL (XEXP (XEXP (x, 0), 1)), 1, 3))
>         {
>           *total = COSTS_N_INSNS (1);
>           return true;
> @@ -2044,7 +2044,6 @@ riscv_rtx_costs (rtx x, machine_mode mod
>             if (!CONST_INT_P (and_rhs))
>               break;
>
> -           rtx ashift_lhs = XEXP (and_lhs, 0);
>             rtx ashift_rhs = XEXP (and_lhs, 1);
>
>             if (!CONST_INT_P (ashift_rhs)