On Wed, Nov 3, 2021 at 12:07 AM Maciej W. Rozycki <ma...@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)
