x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns

In i386.md, neg patterns which set MODE_CC register like

(set (reg:CCC FLAGS_REG)
     (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))

can lead to errors when operand 1 is a constant value.  If FLAGS_REG in

(set (reg:CCC FLAGS_REG)
     (ne:CCC (const_int 2) (const_int 0)))

is set to 1, RTX simplifiers may simplify

(set (reg:SI 93)
     (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))

as

(set (reg:SI 93)
     (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))

which leads to incorrect results since LTU on MODE_CC register isn't the
same as "unsigned less than" in x86 backend.  To prevent RTL optimizers
from setting MODE_CC register to a constant, use UNSPEC_CC_NE to replace
ne:CCC/ne:CCO when setting FLAGS_REG in neg patterns.

gcc/

	PR target/107172
	* config/i386/i386.md (UNSPEC_CC_NE): New.
	Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.

gcc/testsuite/

	PR target/107172
	* gcc.target/i386/pr107172.c: New test.
---
 gcc/config/i386/i386.md                  | 45 +++++++++++++-----------
 gcc/testsuite/gcc.target/i386/pr107172.c | 26 ++++++++++++++
 2 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr107172.c

On Wed, Oct 26, 2022 at 8:59 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> In i386.md, neg patterns which set MODE_CC register like
>
> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
>
> can lead to errors when operand 1 is a constant value.  If FLAGS_REG in
>
> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (const_int 2) (const_int 0)))
>
> is set to 1, RTX simplifiers may simplify
>
> (set (reg:SI 93)
>      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
>
> as
>
> (set (reg:SI 93)
>      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
>
> which leads to incorrect results since LTU on MODE_CC register isn't the
> same as "unsigned less than" in x86 backend.  To prevent RTL optimizers
> from setting MODE_CC register to a constant, use UNSPEC_CC_NE to replace
> ne:CCC/ne:CCO when setting FLAGS_REG in neg patterns.
>
> gcc/
>
>         PR target/107172
>         * config/i386/i386.md (UNSPEC_CC_NE): New.
>         Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.
>
> gcc/testsuite/
>
>         PR target/107172
>         * gcc.target/i386/pr107172.c: New test.

Looking at the PR107172, comments #44 and #45, this patch is a trivial
substitution for an invalid RTX.

So, OK.

Thanks,
Uros.

> ---
>  gcc/config/i386/i386.md                  | 45 +++++++++++++-----------
>  gcc/testsuite/gcc.target/i386/pr107172.c | 26 ++++++++++++++
>  2 files changed, 51 insertions(+), 20 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr107172.c
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index baf1f1f8fa2..aaa678e7314 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -113,6 +113,7 @@ (define_c_enum "unspec" [
>    UNSPEC_PEEPSIB
>    UNSPEC_INSN_FALSE_DEP
>    UNSPEC_SBB
> +  UNSPEC_CC_NE
>
>    ;; For SSE/MMX support:
>    UNSPEC_FIX_NOTRUNC
> @@ -11470,7 +11471,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword"
>    "&& reload_completed"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 0) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 2)
> @@ -11499,7 +11500,8 @@ (define_peephole2
>         (match_operand:SWI48 1 "nonimmediate_gr_operand"))
>     (parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 2 "general_reg_operand") (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 2 "general_reg_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11517,7 +11519,7 @@ (define_peephole2
>     && !reg_mentioned_p (operands[2], operands[1])"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 2) (const_int 0)))
> +         (unspec:CCC [(match_dup 2) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11543,7 +11545,8 @@ (define_peephole2
>       (clobber (reg:CC FLAGS_REG))])
>     (parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 1 "general_reg_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11559,7 +11562,7 @@ (define_peephole2
>    "REGNO (operands[0]) != REGNO (operands[1])"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11635,9 +11638,9 @@ (define_insn "*negsi_2_zext"
>
>  (define_insn "*neg<mode>_ccc_1"
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC
> -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> -         (const_int 0)))
> +       (unspec:CCC
> +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +          (const_int 0)] UNSPEC_CC_NE))
>     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
>         (neg:SWI (match_dup 1)))]
>    ""
> @@ -11647,9 +11650,9 @@ (define_insn "*neg<mode>_ccc_1"
>
>  (define_insn "*neg<mode>_ccc_2"
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC
> -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> -         (const_int 0)))
> +       (unspec:CCC
> +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +          (const_int 0)] UNSPEC_CC_NE))
>     (clobber (match_scratch:SWI 0 "=<r>"))]
>    ""
>    "neg{<imodesuffix>}\t%0"
> @@ -11659,8 +11662,8 @@ (define_insn "*neg<mode>_ccc_2"
>  (define_expand "x86_neg<mode>_ccc"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 1 "register_operand")
> -                 (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 1 "register_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_operand:SWI48 0 "register_operand")
>           (neg:SWI48 (match_dup 1)))])])
>
> @@ -11686,8 +11689,9 @@ (define_insn "*negqi_ext<mode>_2"
>  ;; Negate with jump on overflow.
>  (define_expand "negv<mode>3"
>    [(parallel [(set (reg:CCO FLAGS_REG)
> -                  (ne:CCO (match_operand:SWI 1 "register_operand")
> -                          (match_dup 3)))
> +                  (unspec:CCO
> +                    [(match_operand:SWI 1 "register_operand")
> +                     (match_dup 3)] UNSPEC_CC_NE))
>               (set (match_operand:SWI 0 "register_operand")
>                    (neg:SWI (match_dup 1)))])
>     (set (pc) (if_then_else
> @@ -11703,8 +11707,9 @@ (define_expand "negv<mode>3"
>
>  (define_insn "*negv<mode>3"
>    [(set (reg:CCO FLAGS_REG)
> -       (ne:CCO (match_operand:SWI 1 "nonimmediate_operand" "0")
> -               (match_operand:SWI 2 "const_int_operand")))
> +       (unspec:CCO [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +                    (match_operand:SWI 2 "const_int_operand")]
> +                   UNSPEC_CC_NE))
>     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
>         (neg:SWI (match_dup 1)))]
>    "ix86_unary_operator_ok (NEG, <MODE>mode, operands)
> @@ -11770,7 +11775,7 @@ (define_insn_and_split "*abs<dwi>2_doubleword"
>     "&& 1"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 5)
> @@ -11814,7 +11819,7 @@ (define_insn_and_split "*nabs<dwi>2_doubleword"
>     "&& 1"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 5)
> @@ -21456,7 +21461,7 @@ (define_split
>             (const_int 0))))]
>    ""
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC (match_dup 1) (const_int 0)))
> +       (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>     (set (match_dup 0)
>         (neg:SWI (ltu:SWI (reg:CCC FLAGS_REG) (const_int 0))))])
>
> diff --git a/gcc/testsuite/gcc.target/i386/pr107172.c b/gcc/testsuite/gcc.target/i386/pr107172.c
> new file mode 100644
> index 00000000000..d2c85f3f47c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr107172.c
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1 -ftree-vrp" } */
> +
> +int a, c, d;
> +int
> +main()
> +{
> +  long e = 1;
> +  int f = a = 1;
> +L1:
> +  if (a)
> +    a = 2;
> +  int h = e = ~e;
> +  c = -1;
> +  if (e >= a)
> +    goto L2;
> +  if (-1 > a)
> +    goto L1;
> +  if (a)
> +    f = -1;
> +L2:
> +  d = (-f + d) & h;
> +  if (d)
> +    __builtin_abort();
> +  return 0;
> +}
> --
> 2.37.3
>

On Thu, Oct 27, 2022 at 2:59 AM H.J. Lu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> In i386.md, neg patterns which set MODE_CC register like
>
> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
>
> can lead to errors when operand 1 is a constant value.  If FLAGS_REG in
>
> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (const_int 2) (const_int 0)))
>
> is set to 1, RTX simplifiers may simplify
>
> (set (reg:SI 93)
>      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
>
> as
>
> (set (reg:SI 93)
>      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
>
> which leads to incorrect results since LTU on MODE_CC register isn't the
> same as "unsigned less than" in x86 backend.  To prevent RTL optimizers
> from setting MODE_CC register to a constant, use UNSPEC_CC_NE to replace
> ne:CCC/ne:CCO when setting FLAGS_REG in neg patterns.
>
> gcc/
>
>         PR target/107172
>         * config/i386/i386.md (UNSPEC_CC_NE): New.
>         Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.
>
> gcc/testsuite/
>
>         PR target/107172
>         * gcc.target/i386/pr107172.c: New test.
> ---
>  gcc/config/i386/i386.md                  | 45 +++++++++++++-----------
>  gcc/testsuite/gcc.target/i386/pr107172.c | 26 ++++++++++++++
>  2 files changed, 51 insertions(+), 20 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr107172.c
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index baf1f1f8fa2..aaa678e7314 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -113,6 +113,7 @@ (define_c_enum "unspec" [
>    UNSPEC_PEEPSIB
>    UNSPEC_INSN_FALSE_DEP
>    UNSPEC_SBB
> +  UNSPEC_CC_NE
>
>    ;; For SSE/MMX support:
>    UNSPEC_FIX_NOTRUNC
> @@ -11470,7 +11471,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword"
>    "&& reload_completed"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 0) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 2)
> @@ -11499,7 +11500,8 @@ (define_peephole2
>         (match_operand:SWI48 1 "nonimmediate_gr_operand"))
>     (parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 2 "general_reg_operand") (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 2 "general_reg_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11517,7 +11519,7 @@ (define_peephole2
>     && !reg_mentioned_p (operands[2], operands[1])"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 2) (const_int 0)))
> +         (unspec:CCC [(match_dup 2) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11543,7 +11545,8 @@ (define_peephole2
>       (clobber (reg:CC FLAGS_REG))])
>     (parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 1 "general_reg_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11559,7 +11562,7 @@ (define_peephole2
>    "REGNO (operands[0]) != REGNO (operands[1])"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
>     (parallel
>      [(set (match_dup 0)
> @@ -11635,9 +11638,9 @@ (define_insn "*negsi_2_zext"
>
>  (define_insn "*neg<mode>_ccc_1"
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC
> -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> -         (const_int 0)))
> +       (unspec:CCC
> +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +          (const_int 0)] UNSPEC_CC_NE))
>     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
>         (neg:SWI (match_dup 1)))]
>    ""
> @@ -11647,9 +11650,9 @@ (define_insn "*neg<mode>_ccc_1"
>
>  (define_insn "*neg<mode>_ccc_2"
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC
> -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> -         (const_int 0)))
> +       (unspec:CCC
> +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +          (const_int 0)] UNSPEC_CC_NE))
>     (clobber (match_scratch:SWI 0 "=<r>"))]
>    ""
>    "neg{<imodesuffix>}\t%0"
> @@ -11659,8 +11662,8 @@ (define_insn "*neg<mode>_ccc_2"
>  (define_expand "x86_neg<mode>_ccc"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_operand:SWI48 1 "register_operand")
> -                 (const_int 0)))
> +         (unspec:CCC [(match_operand:SWI48 1 "register_operand")
> +                      (const_int 0)] UNSPEC_CC_NE))
>       (set (match_operand:SWI48 0 "register_operand")
>           (neg:SWI48 (match_dup 1)))])])
>
> @@ -11686,8 +11689,9 @@ (define_insn "*negqi_ext<mode>_2"
>  ;; Negate with jump on overflow.
>  (define_expand "negv<mode>3"
>    [(parallel [(set (reg:CCO FLAGS_REG)
> -                  (ne:CCO (match_operand:SWI 1 "register_operand")
> -                          (match_dup 3)))
> +                  (unspec:CCO
> +                    [(match_operand:SWI 1 "register_operand")
> +                     (match_dup 3)] UNSPEC_CC_NE))
>               (set (match_operand:SWI 0 "register_operand")
>                    (neg:SWI (match_dup 1)))])
>     (set (pc) (if_then_else
> @@ -11703,8 +11707,9 @@ (define_expand "negv<mode>3"
>
>  (define_insn "*negv<mode>3"
>    [(set (reg:CCO FLAGS_REG)
> -       (ne:CCO (match_operand:SWI 1 "nonimmediate_operand" "0")
> -               (match_operand:SWI 2 "const_int_operand")))
> +       (unspec:CCO [(match_operand:SWI 1 "nonimmediate_operand" "0")
> +                    (match_operand:SWI 2 "const_int_operand")]
> +                   UNSPEC_CC_NE))
>     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
>         (neg:SWI (match_dup 1)))]
>    "ix86_unary_operator_ok (NEG, <MODE>mode, operands)
> @@ -11770,7 +11775,7 @@ (define_insn_and_split "*abs<dwi>2_doubleword"
>     "&& 1"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 5)
> @@ -11814,7 +11819,7 @@ (define_insn_and_split "*nabs<dwi>2_doubleword"
>     "&& 1"
>    [(parallel
>      [(set (reg:CCC FLAGS_REG)
> -         (ne:CCC (match_dup 1) (const_int 0)))
> +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
>       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
>     (parallel
>      [(set (match_dup 5)
> @@ -21456,7 +21461,7 @@ (define_split
>             (const_int 0))))]
>    ""
>    [(set (reg:CCC FLAGS_REG)
> -       (ne:CCC (match_dup 1) (const_int 0)))
> +       (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
Do we have a define_insn for this? It looks like a setcc to me.
>     (set (match_dup 0)
>         (neg:SWI (ltu:SWI (reg:CCC FLAGS_REG) (const_int 0))))])
>
> diff --git a/gcc/testsuite/gcc.target/i386/pr107172.c b/gcc/testsuite/gcc.target/i386/pr107172.c
> new file mode 100644
> index 00000000000..d2c85f3f47c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr107172.c
> @@ -0,0 +1,26 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1 -ftree-vrp" } */
> +
> +int a, c, d;
> +int
> +main()
> +{
> +  long e = 1;
> +  int f = a = 1;
> +L1:
> +  if (a)
> +    a = 2;
> +  int h = e = ~e;
> +  c = -1;
> +  if (e >= a)
> +    goto L2;
> +  if (-1 > a)
> +    goto L1;
> +  if (a)
> +    f = -1;
> +L2:
> +  d = (-f + d) & h;
> +  if (d)
> +    __builtin_abort();
> +  return 0;
> +}
> --
> 2.37.3
>

On Fri, Oct 28, 2022 at 1:56 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, Oct 27, 2022 at 2:59 AM H.J. Lu via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > In i386.md, neg patterns which set MODE_CC register like
> >
> > (set (reg:CCC FLAGS_REG)
> >      (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> >
> > can lead to errors when operand 1 is a constant value.  If FLAGS_REG in
> >
> > (set (reg:CCC FLAGS_REG)
> >      (ne:CCC (const_int 2) (const_int 0)))
> >
> > is set to 1, RTX simplifiers may simplify
> >
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
> >
> > as
> >
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
> >
> > which leads to incorrect results since LTU on MODE_CC register isn't the
> > same as "unsigned less than" in x86 backend.  To prevent RTL optimizers
> > from setting MODE_CC register to a constant, use UNSPEC_CC_NE to replace
> > ne:CCC/ne:CCO when setting FLAGS_REG in neg patterns.
> >
> > gcc/
> >
> >         PR target/107172
> >         * config/i386/i386.md (UNSPEC_CC_NE): New.
> >         Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.
> >
> > gcc/testsuite/
> >
> >         PR target/107172
> >         * gcc.target/i386/pr107172.c: New test.
> > ---
> >  gcc/config/i386/i386.md                  | 45 +++++++++++++-----------
> >  gcc/testsuite/gcc.target/i386/pr107172.c | 26 ++++++++++++++
> >  2 files changed, 51 insertions(+), 20 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr107172.c
> >
> > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> > index baf1f1f8fa2..aaa678e7314 100644
> > --- a/gcc/config/i386/i386.md
> > +++ b/gcc/config/i386/i386.md
> > @@ -113,6 +113,7 @@ (define_c_enum "unspec" [
> >    UNSPEC_PEEPSIB
> >    UNSPEC_INSN_FALSE_DEP
> >    UNSPEC_SBB
> > +  UNSPEC_CC_NE
> >
> >    ;; For SSE/MMX support:
> >    UNSPEC_FIX_NOTRUNC
> > @@ -11470,7 +11471,7 @@ (define_insn_and_split "*neg<dwi>2_doubleword"
> >    "&& reload_completed"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_dup 1) (const_int 0)))
> > +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 0) (neg:DWIH (match_dup 1)))])
> >     (parallel
> >      [(set (match_dup 2)
> > @@ -11499,7 +11500,8 @@ (define_peephole2
> >         (match_operand:SWI48 1 "nonimmediate_gr_operand"))
> >     (parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_operand:SWI48 2 "general_reg_operand") (const_int 0)))
> > +         (unspec:CCC [(match_operand:SWI48 2 "general_reg_operand")
> > +                      (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
> >     (parallel
> >      [(set (match_dup 0)
> > @@ -11517,7 +11519,7 @@ (define_peephole2
> >     && !reg_mentioned_p (operands[2], operands[1])"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_dup 2) (const_int 0)))
> > +         (unspec:CCC [(match_dup 2) (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 2) (neg:SWI48 (match_dup 2)))])
> >     (parallel
> >      [(set (match_dup 0)
> > @@ -11543,7 +11545,8 @@ (define_peephole2
> >       (clobber (reg:CC FLAGS_REG))])
> >     (parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> > +         (unspec:CCC [(match_operand:SWI48 1 "general_reg_operand")
> > +                      (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
> >     (parallel
> >      [(set (match_dup 0)
> > @@ -11559,7 +11562,7 @@ (define_peephole2
> >    "REGNO (operands[0]) != REGNO (operands[1])"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_dup 1) (const_int 0)))
> > +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 1) (neg:SWI48 (match_dup 1)))])
> >     (parallel
> >      [(set (match_dup 0)
> > @@ -11635,9 +11638,9 @@ (define_insn "*negsi_2_zext"
> >
> >  (define_insn "*neg<mode>_ccc_1"
> >    [(set (reg:CCC FLAGS_REG)
> > -       (ne:CCC
> > -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> > -         (const_int 0)))
> > +       (unspec:CCC
> > +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> > +          (const_int 0)] UNSPEC_CC_NE))
> >     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
> >         (neg:SWI (match_dup 1)))]
> >    ""
> > @@ -11647,9 +11650,9 @@ (define_insn "*neg<mode>_ccc_1"
> >
> >  (define_insn "*neg<mode>_ccc_2"
> >    [(set (reg:CCC FLAGS_REG)
> > -       (ne:CCC
> > -         (match_operand:SWI 1 "nonimmediate_operand" "0")
> > -         (const_int 0)))
> > +       (unspec:CCC
> > +         [(match_operand:SWI 1 "nonimmediate_operand" "0")
> > +          (const_int 0)] UNSPEC_CC_NE))
> >     (clobber (match_scratch:SWI 0 "=<r>"))]
> >    ""
> >    "neg{<imodesuffix>}\t%0"
> > @@ -11659,8 +11662,8 @@ (define_insn "*neg<mode>_ccc_2"
> >  (define_expand "x86_neg<mode>_ccc"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_operand:SWI48 1 "register_operand")
> > -                 (const_int 0)))
> > +         (unspec:CCC [(match_operand:SWI48 1 "register_operand")
> > +                      (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_operand:SWI48 0 "register_operand")
> >           (neg:SWI48 (match_dup 1)))])])
> >
> > @@ -11686,8 +11689,9 @@ (define_insn "*negqi_ext<mode>_2"
> >  ;; Negate with jump on overflow.
> >  (define_expand "negv<mode>3"
> >    [(parallel [(set (reg:CCO FLAGS_REG)
> > -                  (ne:CCO (match_operand:SWI 1 "register_operand")
> > -                          (match_dup 3)))
> > +                  (unspec:CCO
> > +                    [(match_operand:SWI 1 "register_operand")
> > +                     (match_dup 3)] UNSPEC_CC_NE))
> >               (set (match_operand:SWI 0 "register_operand")
> >                    (neg:SWI (match_dup 1)))])
> >     (set (pc) (if_then_else
> > @@ -11703,8 +11707,9 @@ (define_expand "negv<mode>3"
> >
> >  (define_insn "*negv<mode>3"
> >    [(set (reg:CCO FLAGS_REG)
> > -       (ne:CCO (match_operand:SWI 1 "nonimmediate_operand" "0")
> > -               (match_operand:SWI 2 "const_int_operand")))
> > +       (unspec:CCO [(match_operand:SWI 1 "nonimmediate_operand" "0")
> > +                    (match_operand:SWI 2 "const_int_operand")]
> > +                   UNSPEC_CC_NE))
> >     (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
> >         (neg:SWI (match_dup 1)))]
> >    "ix86_unary_operator_ok (NEG, <MODE>mode, operands)
> > @@ -11770,7 +11775,7 @@ (define_insn_and_split "*abs<dwi>2_doubleword"
> >     "&& 1"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_dup 1) (const_int 0)))
> > +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
> >     (parallel
> >      [(set (match_dup 5)
> > @@ -11814,7 +11819,7 @@ (define_insn_and_split "*nabs<dwi>2_doubleword"
> >     "&& 1"
> >    [(parallel
> >      [(set (reg:CCC FLAGS_REG)
> > -         (ne:CCC (match_dup 1) (const_int 0)))
> > +         (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
> >       (set (match_dup 2) (neg:DWIH (match_dup 1)))])
> >     (parallel
> >      [(set (match_dup 5)
> > @@ -21456,7 +21461,7 @@ (define_split
> >             (const_int 0))))]


> >    [(set (reg:CCC FLAGS_REG)
> > -       (ne:CCC (match_dup 1) (const_int 0)))
> > +       (unspec:CCC [(match_dup 1) (const_int 0)] UNSPEC_CC_NE))
> Do we have a define_insn for this? It looks like a setcc to me.
It's not, nevermind.

> >     (set (match_dup 0)
> >         (neg:SWI (ltu:SWI (reg:CCC FLAGS_REG) (const_int 0))))])
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr107172.c b/gcc/testsuite/gcc.target/i386/pr107172.c
> > new file mode 100644
> > index 00000000000..d2c85f3f47c
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr107172.c
> > @@ -0,0 +1,26 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-O1 -ftree-vrp" } */
> > +
> > +int a, c, d;
> > +int
> > +main()
> > +{
> > +  long e = 1;
> > +  int f = a = 1;
> > +L1:
> > +  if (a)
> > +    a = 2;
> > +  int h = e = ~e;
> > +  c = -1;
> > +  if (e >= a)
> > +    goto L2;
> > +  if (-1 > a)
> > +    goto L1;
> > +  if (a)
> > +    f = -1;
> > +L2:
> > +  d = (-f + d) & h;
> > +  if (d)
> > +    __builtin_abort();
> > +  return 0;
> > +}
> > --
> > 2.37.3
> >
>
>
> --
> BR,
> Hongtao

> (set (reg:SI 93)
>      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
> 
> as
> 
> (set (reg:SI 93)
>      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
> 
> which leads to incorrect results since LTU on MODE_CC register isn't the
> same as "unsigned less than" in x86 backend.

That's not specific to the x86 back-end, i.e. it's a generic caveat.

> 	PR target/107172
> 	* config/i386/i386.md (UNSPEC_CC_NE): New.
> 	Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.

FWIW the SPARC back-end uses a COMPARE instead of an UNSPEC here.

On Fri, Oct 28, 2022 at 1:35 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
> >
> > as
> >
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
> >
> > which leads to incorrect results since LTU on MODE_CC register isn't the
> > same as "unsigned less than" in x86 backend.
>
> That's not specific to the x86 back-end, i.e. it's a generic caveat.
>
> >       PR target/107172
> >       * config/i386/i386.md (UNSPEC_CC_NE): New.
> >       Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.
>
> FWIW the SPARC back-end uses a COMPARE instead of an UNSPEC here.

COMPARE may also set CC register to a constant when both operands are
known constants.

> COMPARE may also set CC register to a constant when both operands are
> known constants.

No, a COMPARE is never evaluated alone, only the CC user may be evaluated.

On Wed, Oct 26, 2022 at 11:58:57AM -0700, H.J. Lu via Gcc-patches wrote:
> In i386.md, neg patterns which set MODE_CC register like
> 
> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> 
> can lead to errors when operand 1 is a constant value.  If FLAGS_REG in

But it cannot be.  general_reg_operand will not allow that:
===
(define_predicate "general_reg_operand"
  (and (match_code "reg")
       (match_test "GENERAL_REGNO_P (REGNO (op))")))
===

> (set (reg:CCC FLAGS_REG)
>      (ne:CCC (const_int 2) (const_int 0)))
> 
> is set to 1, RTX simplifiers may simplify

"is set to 1"?  Do you mean you do something like
  (set (regs FLAGS_REG) (const_int 1))
?  That is invalid RTL, as I've said tens of time in the last few weeks.

> which leads to incorrect results since LTU on MODE_CC register isn't the
> same as "unsigned less than" in x86 backend.

The special notation
  (ltu (reg:CC) (const_int 0))
is not about comparing anything to 0, but simply means "did the
comparison-like thing that set that reg say ltu was true".

> To prevent RTL optimizers
> from setting MODE_CC register to a constant, use UNSPEC_CC_NE to replace
> ne:CCC/ne:CCO when setting FLAGS_REG in neg patterns.

This is an indirect workaround, nothing more.  The unspec will naturally
not be folded to anything else (unless you arrange for that yourself),
there is nothing the generic code knows about the semantics of any
unspec after all.

AFIACS there is no way to express overflow in a CC, but an unspec can
help, sure.  You need to fix the setter side as well though.


Segher

Hi!

On Fri, Oct 28, 2022 at 10:35:03AM +0200, Eric Botcazou via Gcc-patches wrote:
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (reg:CCC 17 flags) (const_int 0 [0]))))
> > 
> > as
> > 
> > (set (reg:SI 93)
> >      (neg:SI (ltu:SI (const_int 1) (const_int 0 [0]))))
> > 
> > which leads to incorrect results since LTU on MODE_CC register isn't the
> > same as "unsigned less than" in x86 backend.
> 
> That's not specific to the x86 back-end, i.e. it's a generic caveat.

A MODE_CC reg can never be "const_int 1".  That is total garbage.  It
cannot work.  It would mean all of
  (eq (reg:CC) (const_int 0))
  (lt (reg:CC) (const_int 0))
  (gt (reg:CC) (const_int 0))
  (ne (reg:CC) (const_int 0))
  (ge (reg:CC) (const_int 0))
  (le (reg:CC) (const_int 0))
(and more) are simultaneously true.

> > 	PR target/107172
> > 	* config/i386/i386.md (UNSPEC_CC_NE): New.
> > 	Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns.
> 
> FWIW the SPARC back-end uses a COMPARE instead of an UNSPEC here.

You mean in CCV?  That works yes, but only because (or if) the setter
and getter of the CC reg both use CCV (so never use any other flag at
the same time; CCV has an empty intersection with all other CC modes).


Segher

> You mean in CCV?  That works yes, but only because (or if) the setter
> and getter of the CC reg both use CCV (so never use any other flag at
> the same time; CCV has an empty intersection with all other CC modes).

We're talking about CCC here AFAIK, i.e. the carry, not CCV.

On Fri, Oct 28, 2022 at 2:34 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Oct 26, 2022 at 11:58:57AM -0700, H.J. Lu via Gcc-patches wrote:
> > In i386.md, neg patterns which set MODE_CC register like
> >
> > (set (reg:CCC FLAGS_REG)
> >      (ne:CCC (match_operand:SWI48 1 "general_reg_operand") (const_int 0)))
> >
> > can lead to errors when operand 1 is a constant value.  If FLAGS_REG in
>
> But it cannot be.  general_reg_operand will not allow that:
> ===
> (define_predicate "general_reg_operand"
>   (and (match_code "reg")
>        (match_test "GENERAL_REGNO_P (REGNO (op))")))
> ===
>
> > (set (reg:CCC FLAGS_REG)
> >      (ne:CCC (const_int 2) (const_int 0)))
> >
> > is set to 1, RTX simplifiers may simplify
>

Here is another example:

(define_insn "*neg<mode>_ccc_1"
  [(set (reg:CCC FLAGS_REG)
    (ne:CCC
      (match_operand:SWI 1 "nonimmediate_operand" "0")
      (const_int 0)))
   (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m")
    (neg:SWI (match_dup 1)))]
  ""
  "neg{<imodesuffix>}\t%0"
  [(set_attr "type" "negnot")
   (set_attr "mode" "<MODE>")])

Operand 1 can be a known value.

H.J.

On Fri, Oct 28, 2022 at 11:55:35PM +0200, Eric Botcazou wrote:
> > You mean in CCV?  That works yes, but only because (or if) the setter
> > and getter of the CC reg both use CCV (so never use any other flag at
> > the same time; CCV has an empty intersection with all other CC modes).
> 
> We're talking about CCC here AFAIK, i.e. the carry, not CCV.

Yes.  But it is all the same: neither signed overflow nor unsigned
overflow (of an addition, say) can be described as the result of an
RTL comparison.

The point is that all of this is put completely outside of all other
MODE_CC handling, and only works because of that.  And a small
modification to the backend, completely elsewhere, can make that house
of cards collapse.  It is much more robust to use a different relation,
not EQ, to decribe this.  Something with an unspec is fine.

But what the sparc backend does does work.


Segher

> Yes.  But it is all the same: neither signed overflow nor unsigned
> overflow (of an addition, say) can be described as the result of an
> RTL comparison.

I disagree, see for example the implementation of the addvdi4_sp3 pattern (for 
which we indeed use an UNSPEC) and of the uaddvdi4_sp32 pattern (for which we 
describe the overflow with a COMPARE) in the SPARC back-end.  And that's even 
simpler for an unsigned subtraction, where we do not need a special CC mode.

Sure there is a technical difficulty for unsigned negation because of the 
canonicalization rules, hence the trick used in the SPARC back-end, but 
unsigned overflow is much easier to deal with than signed overflow.