Re: [PATCH] x86: Fix cmov cost model issue [PR109549]

[Uros Bizjak]

On Mon, May 6, 2024 at 5:20 AM Hongtao Liu  wrote:
>
> CC uros.
>
> On Mon, May 6, 2024 at 11:03 AM Kong, Lingling  
> wrote:
> >
> > Hi,
> > (if_then_else:SI (eq (reg:CCZ 17 flags)
> > (const_int 0 [0]))
> > (reg/v:SI 101 [ e ])
> > (reg:SI 102))
> > The cost is 8 for the rtx, the cost for
> > (eq (reg:CCZ 17 flags) (const_int 0 [0])) is 4, but this is just an 
> > operator do not need to compute it's cost in cmov.
> It looks like a reasonable change to me, for cmov, the first operand
> of if_then_else is not a mask.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu.
> > OK for trunk?
> >
> > gcc/ChangeLog:
> >
> > PR target/109549
> > * config/i386/i386.cc (ix86_rtx_costs): The XEXP (x, 0) for cmov
> > is an operator do not need to compute cost.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/i386/cmov6.c: Fixed.

OK.

BTW: I'd like to point out PR85559 [1] that collects some persistent
issues with x86 CMOV insn, especially [2].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=cmov
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56309

Uros.

> > ---
> >  gcc/config/i386/i386.cc   | 2 +-
> >  gcc/testsuite/gcc.target/i386/cmov6.c | 5 +
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 
> > 4d6b2b98761..59b4ce3bfbf 100644
> > --- a/gcc/config/i386/i386.cc
> > +++ b/gcc/config/i386/i386.cc
> > @@ -22237,7 +22237,7 @@ ix86_rtx_costs (rtx x, machine_mode mode, int 
> > outer_code_i, int opno,
> > {
> >   /* cmov.  */
> >   *total = COSTS_N_INSNS (1);
> > - if (!REG_P (XEXP (x, 0)))
> > + if (!COMPARISON_P (XEXP (x, 0)) && !REG_P (XEXP (x, 0)))
> > *total += rtx_cost (XEXP (x, 0), mode, code, 0, speed);
> >   if (!REG_P (XEXP (x, 1)))
> > *total += rtx_cost (XEXP (x, 1), mode, code, 1, speed); diff 
> > --git a/gcc/testsuite/gcc.target/i386/cmov6.c 
> > b/gcc/testsuite/gcc.target/i386/cmov6.c
> > index 5111c8a9099..535326e4c2a 100644
> > --- a/gcc/testsuite/gcc.target/i386/cmov6.c
> > +++ b/gcc/testsuite/gcc.target/i386/cmov6.c
> > @@ -1,9 +1,6 @@
> >  /* { dg-do compile } */
> >  /* { dg-options "-O2 -march=k8" } */
> > -/* if-converting this sequence would require two cmov
> > -   instructions and seems to always cost more independent
> > -   of the TUNE_ONE_IF_CONV setting.  */
> > -/* { dg-final { scan-assembler-not "cmov\[^6\]" } } */
> > +/* { dg-final { scan-assembler "cmov\[^6\]" } } */
> >
> >  /* Verify that blocks are converted to conditional moves.  */  extern int 
> > bar (int, int);
> > --
> > 2.31.1
> >
>
>
> --
> BR,
> Hongtao

Re: [PATCH] x86: Fix cmov cost model issue [PR109549]

[Hongtao Liu]

CC uros.

On Mon, May 6, 2024 at 11:03 AM Kong, Lingling  wrote:
>
> Hi,
> (if_then_else:SI (eq (reg:CCZ 17 flags)
> (const_int 0 [0]))
> (reg/v:SI 101 [ e ])
> (reg:SI 102))
> The cost is 8 for the rtx, the cost for
> (eq (reg:CCZ 17 flags) (const_int 0 [0])) is 4, but this is just an operator 
> do not need to compute it's cost in cmov.
It looks like a reasonable change to me, for cmov, the first operand
of if_then_else is not a mask.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK for trunk?
>
> gcc/ChangeLog:
>
> PR target/109549
> * config/i386/i386.cc (ix86_rtx_costs): The XEXP (x, 0) for cmov
> is an operator do not need to compute cost.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/i386/cmov6.c: Fixed.
> ---
>  gcc/config/i386/i386.cc   | 2 +-
>  gcc/testsuite/gcc.target/i386/cmov6.c | 5 +
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 
> 4d6b2b98761..59b4ce3bfbf 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -22237,7 +22237,7 @@ ix86_rtx_costs (rtx x, machine_mode mode, int 
> outer_code_i, int opno,
> {
>   /* cmov.  */
>   *total = COSTS_N_INSNS (1);
> - if (!REG_P (XEXP (x, 0)))
> + if (!COMPARISON_P (XEXP (x, 0)) && !REG_P (XEXP (x, 0)))
> *total += rtx_cost (XEXP (x, 0), mode, code, 0, speed);
>   if (!REG_P (XEXP (x, 1)))
> *total += rtx_cost (XEXP (x, 1), mode, code, 1, speed); diff 
> --git a/gcc/testsuite/gcc.target/i386/cmov6.c 
> b/gcc/testsuite/gcc.target/i386/cmov6.c
> index 5111c8a9099..535326e4c2a 100644
> --- a/gcc/testsuite/gcc.target/i386/cmov6.c
> +++ b/gcc/testsuite/gcc.target/i386/cmov6.c
> @@ -1,9 +1,6 @@
>  /* { dg-do compile } */
>  /* { dg-options "-O2 -march=k8" } */
> -/* if-converting this sequence would require two cmov
> -   instructions and seems to always cost more independent
> -   of the TUNE_ONE_IF_CONV setting.  */
> -/* { dg-final { scan-assembler-not "cmov\[^6\]" } } */
> +/* { dg-final { scan-assembler "cmov\[^6\]" } } */
>
>  /* Verify that blocks are converted to conditional moves.  */  extern int 
> bar (int, int);
> --
> 2.31.1
>


-- 
BR,
Hongtao

[PATCH] x86: Fix cmov cost model issue [PR109549]

[Kong, Lingling]

Hi,
(if_then_else:SI (eq (reg:CCZ 17 flags)
(const_int 0 [0]))
(reg/v:SI 101 [ e ])
(reg:SI 102))
The cost is 8 for the rtx, the cost for
(eq (reg:CCZ 17 flags) (const_int 0 [0])) is 4, but this is just an operator do 
not need to compute it's cost in cmov.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

gcc/ChangeLog:

PR target/109549
* config/i386/i386.cc (ix86_rtx_costs): The XEXP (x, 0) for cmov
is an operator do not need to compute cost.

gcc/testsuite/ChangeLog:

* gcc.target/i386/cmov6.c: Fixed.
---
 gcc/config/i386/i386.cc   | 2 +-
 gcc/testsuite/gcc.target/i386/cmov6.c | 5 +
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 
4d6b2b98761..59b4ce3bfbf 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -22237,7 +22237,7 @@ ix86_rtx_costs (rtx x, machine_mode mode, int 
outer_code_i, int opno,
{
  /* cmov.  */
  *total = COSTS_N_INSNS (1);
- if (!REG_P (XEXP (x, 0)))
+ if (!COMPARISON_P (XEXP (x, 0)) && !REG_P (XEXP (x, 0)))
*total += rtx_cost (XEXP (x, 0), mode, code, 0, speed);
  if (!REG_P (XEXP (x, 1)))
*total += rtx_cost (XEXP (x, 1), mode, code, 1, speed); diff --git 
a/gcc/testsuite/gcc.target/i386/cmov6.c b/gcc/testsuite/gcc.target/i386/cmov6.c
index 5111c8a9099..535326e4c2a 100644
--- a/gcc/testsuite/gcc.target/i386/cmov6.c
+++ b/gcc/testsuite/gcc.target/i386/cmov6.c
@@ -1,9 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -march=k8" } */
-/* if-converting this sequence would require two cmov
-   instructions and seems to always cost more independent
-   of the TUNE_ONE_IF_CONV setting.  */
-/* { dg-final { scan-assembler-not "cmov\[^6\]" } } */
+/* { dg-final { scan-assembler "cmov\[^6\]" } } */
 
 /* Verify that blocks are converted to conditional moves.  */  extern int bar 
(int, int);
--
2.31.1