* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-26 18:58 [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns H.J. Lu
@ 2022-10-27 6:06 ` Uros Bizjak
2022-10-28 5:56 ` Hongtao Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Uros Bizjak @ 2022-10-27 6:06 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-26 18:58 [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns H.J. Lu
2022-10-27 6:06 ` Uros Bizjak
@ 2022-10-28 5:56 ` Hongtao Liu
2022-10-28 6:08 ` Hongtao Liu
2022-10-28 8:35 ` Eric Botcazou
2022-10-28 21:32 ` Segher Boessenkool
3 siblings, 1 reply; 13+ messages in thread
From: Hongtao Liu @ 2022-10-28 5:56 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches, Uros Bizjak
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
>
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 5:56 ` Hongtao Liu
@ 2022-10-28 6:08 ` Hongtao Liu
0 siblings, 0 replies; 13+ messages in thread
From: Hongtao Liu @ 2022-10-28 6:08 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches, Uros Bizjak
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
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-26 18:58 [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns H.J. Lu
2022-10-27 6:06 ` Uros Bizjak
2022-10-28 5:56 ` Hongtao Liu
@ 2022-10-28 8:35 ` Eric Botcazou
2022-10-28 15:51 ` H.J. Lu
2022-10-28 21:43 ` Segher Boessenkool
2022-10-28 21:32 ` Segher Boessenkool
3 siblings, 2 replies; 13+ messages in thread
From: Eric Botcazou @ 2022-10-28 8:35 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches, Uros Bizjak
> (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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 8:35 ` Eric Botcazou
@ 2022-10-28 15:51 ` H.J. Lu
2022-10-28 17:58 ` Eric Botcazou
2022-10-28 21:43 ` Segher Boessenkool
1 sibling, 1 reply; 13+ messages in thread
From: H.J. Lu @ 2022-10-28 15:51 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Uros Bizjak
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.
--
H.J.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 8:35 ` Eric Botcazou
2022-10-28 15:51 ` H.J. Lu
@ 2022-10-28 21:43 ` Segher Boessenkool
2022-10-28 21:55 ` Eric Botcazou
1 sibling, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2022-10-28 21:43 UTC (permalink / raw)
To: Eric Botcazou; +Cc: H.J. Lu, gcc-patches
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 21:43 ` Segher Boessenkool
@ 2022-10-28 21:55 ` Eric Botcazou
2022-11-01 20:34 ` Segher Boessenkool
0 siblings, 1 reply; 13+ messages in thread
From: Eric Botcazou @ 2022-10-28 21:55 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches, gcc-patches
> 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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 21:55 ` Eric Botcazou
@ 2022-11-01 20:34 ` Segher Boessenkool
2022-11-01 21:28 ` Eric Botcazou
0 siblings, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2022-11-01 20:34 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-11-01 20:34 ` Segher Boessenkool
@ 2022-11-01 21:28 ` Eric Botcazou
0 siblings, 0 replies; 13+ messages in thread
From: Eric Botcazou @ 2022-11-01 21:28 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
> 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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-26 18:58 [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns H.J. Lu
` (2 preceding siblings ...)
2022-10-28 8:35 ` Eric Botcazou
@ 2022-10-28 21:32 ` Segher Boessenkool
2022-10-28 22:28 ` H.J. Lu
3 siblings, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2022-10-28 21:32 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches, Uros Bizjak
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] x86: Replace ne:CCC/ne:CCO with UNSPEC_CC_NE in neg patterns
2022-10-28 21:32 ` Segher Boessenkool
@ 2022-10-28 22:28 ` H.J. Lu
0 siblings, 0 replies; 13+ messages in thread
From: H.J. Lu @ 2022-10-28 22:28 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches, Uros Bizjak
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.
^ permalink raw reply [flat|nested] 13+ messages in thread