* [PATCH] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
@ 2021-11-01 12:40 Maciej W. Rozycki
2021-11-02 15:03 ` Kito Cheng
0 siblings, 1 reply; 5+ messages in thread
From: Maciej W. Rozycki @ 2021-11-01 12:40 UTC (permalink / raw)
To: gcc-patches; +Cc: Kito Cheng, Palmer Dabbelt, Andrew Waterman, Jim Wilson
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 removing an incorrect REG_P check applied to a constant expression
and getting rid of the unused variable.
gcc/
* config/riscv/riscv.c (riscv_rtx_costs): Remove a REG_P check
and an unused local variable with shNadd/shNadd.uw pattern
handling.
---
Hi,
As described above and I guess almost obvious -- I gather the code was
only verified with a `-Wno-error' build and the handling of the shNadd
pattern has not been actually covered owing to this bug making the
condition impossible to match.
OK to apply then?
Maciej
---
gcc/config/riscv/riscv.c | 2 --
1 file changed, 2 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
@@ -2013,7 +2013,6 @@ riscv_rtx_costs (rtx x, machine_mode mod
&& ((!TARGET_64BIT && (mode == SImode)) ||
(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))
{
@@ -2044,7 +2043,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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
2021-11-01 12:40 [PATCH] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model Maciej W. Rozycki
@ 2021-11-02 15:03 ` Kito Cheng
2021-11-02 16:06 ` [committed v2] " Maciej W. Rozycki
0 siblings, 1 reply; 5+ messages in thread
From: Kito Cheng @ 2021-11-02 15:03 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: GCC Patches, Andrew Waterman
Hi Maciej:
On Mon, Nov 1, 2021 at 8:41 PM 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 removing an incorrect REG_P check applied to a constant expression
> and getting rid of the unused variable.
>
> gcc/
> * config/riscv/riscv.c (riscv_rtx_costs): Remove a REG_P check
> and an unused local variable with shNadd/shNadd.uw pattern
> handling.
> ---
> Hi,
>
> As described above and I guess almost obvious -- I gather the code was
> only verified with a `-Wno-error' build and the handling of the shNadd
> pattern has not been actually covered owing to this bug making the
> condition impossible to match.
>
> OK to apply then?
>
> Maciej
> ---
> gcc/config/riscv/riscv.c | 2 --
> 1 file changed, 2 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
> @@ -2013,7 +2013,6 @@ riscv_rtx_costs (rtx x, machine_mode mod
> && ((!TARGET_64BIT && (mode == SImode)) ||
> (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))
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")))]
Otherwise LGTM, feel free to commit once you address this issue.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
* [committed v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
2021-11-02 15:03 ` Kito Cheng
@ 2021-11-02 16:06 ` Maciej W. Rozycki
2021-11-02 16:43 ` Kito Cheng
0 siblings, 1 reply; 5+ messages in thread
From: Maciej W. Rozycki @ 2021-11-02 16:06 UTC (permalink / raw)
To: Kito Cheng; +Cc: GCC Patches, Andrew Waterman
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?
> Otherwise LGTM, feel free to commit once you address this issue.
Rebuilt for verification and committed as shown. Thank you for your
review.
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [committed v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
2021-11-02 16:06 ` [committed v2] " Maciej W. Rozycki
@ 2021-11-02 16:43 ` Kito Cheng
2021-11-03 17:18 ` Maciej W. Rozycki
0 siblings, 1 reply; 5+ messages in thread
From: Kito Cheng @ 2021-11-02 16:43 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: GCC Patches, Andrew Waterman
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [committed v2] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model
2021-11-02 16:43 ` Kito Cheng
@ 2021-11-03 17:18 ` Maciej W. Rozycki
0 siblings, 0 replies; 5+ messages in thread
From: Maciej W. Rozycki @ 2021-11-03 17:18 UTC (permalink / raw)
To: Kito Cheng; +Cc: GCC Patches, Andrew Waterman
On Wed, 3 Nov 2021, Kito Cheng wrote:
> > 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.
Fair enough. Note that intermediate dumps can be used for automatic
verification too, with `scan-tree-dump', `scan-rtl-dump', etc. (I guess
you probably knew that already). I know that writing a good test case
can be tricky.
Maciej
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-03 17:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-01 12:40 [PATCH] RISC-V: Fix build errors with shNadd/shNadd.uw patterns in zba cost model Maciej W. Rozycki
2021-11-02 15:03 ` Kito Cheng
2021-11-02 16:06 ` [committed v2] " Maciej W. Rozycki
2021-11-02 16:43 ` Kito Cheng
2021-11-03 17:18 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).