diff mbox series

Add define_insn_and_split for combine to detect x < 123U ? -1 : 0 (PR target/88425)

Message ID 20181211072721.GK12380@tucnak
State New
Headers show
Series Add define_insn_and_split for combine to detect x < 123U ? -1 : 0 (PR target/88425) | expand

Commit Message

Jakub Jelinek Dec. 11, 2018, 7:27 a.m. UTC 
Hi!

For the following testcase (x < 123U ? -1U : 0) we emit worse code than for
(x < y ? -1U : 0).  That is because the generic code canonicalizes x < 123U
comparisons into x <= 122U.  For this particular case, we want LTU though,
as that sets carry and we can then just sbb.

The following patch adds an insn with splitter for this.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/88425
	* config/i386/i386.md (*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>):
	New define_insn_and_split.

	* gcc.target/i386/pr88425.c: New test.


	Jakub

Comments

Uros Bizjak Dec. 11, 2018, 7:44 a.m. UTC | #1 
On Tue, Dec 11, 2018 at 8:27 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> For the following testcase (x < 123U ? -1U : 0) we emit worse code than for
> (x < y ? -1U : 0).  That is because the generic code canonicalizes x < 123U
> comparisons into x <= 122U.  For this particular case, we want LTU though,
> as that sets carry and we can then just sbb.
>
> The following patch adds an insn with splitter for this.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-12-11  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/88425
>         * config/i386/i386.md (*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>):
>         New define_insn_and_split.
>
>         * gcc.target/i386/pr88425.c: New test.
>
> --- gcc/config/i386/i386.md.jj  2018-11-22 10:40:31.179683319 +0100
> +++ gcc/config/i386/i386.md     2018-12-10 11:24:49.785830186 +0100
> @@ -17195,6 +17195,24 @@ (define_insn "*x86_mov<mode>cc_0_m1_neg"
>     (set_attr "mode" "<MODE>")
>     (set_attr "length_immediate" "0")])
>
> +(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
> +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> +       (neg:SWI48
> +         (leu:SWI48
> +           (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
> +           (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))

You can use const_int_operand predicate with "n" constraint here.

> +   (clobber (reg:CC FLAGS_REG))]
> +  "CONST_INT_P (operands[2])
> +   && INTVAL (operands[2]) != -1
> +   && INTVAL (operands[2]) != 2147483647"

Can UINTVAL be used here?

Uros.

> +  "#"
> +  ""
> +  [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2)))
> +   (parallel [(set (match_dup 0)
> +                  (neg:SWI48 (ltu:SWI48 (reg:CC FLAGS_REG) (const_int 0))))
> +             (clobber (reg:CC FLAGS_REG))])]
> +  "operands[2] = GEN_INT (INTVAL (operands[2]) + 1);")
> +
>  (define_insn "*mov<mode>cc_noc"
>    [(set (match_operand:SWI248 0 "register_operand" "=r,r")
>         (if_then_else:SWI248 (match_operator 1 "ix86_comparison_operator"
> --- gcc/testsuite/gcc.target/i386/pr88425.c.jj  2018-12-10 11:31:35.507196453 +0100
> +++ gcc/testsuite/gcc.target/i386/pr88425.c     2018-12-10 11:31:17.231495274 +0100
> @@ -0,0 +1,53 @@
> +/* PR target/88425 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -masm=att" } */
> +/* { dg-final { scan-assembler-times "sbb\[lq]\[ \t]" 8 } } */
> +/* { dg-final { scan-assembler-not "setbe\[ \t]" } } */
> +
> +unsigned long
> +f1 (unsigned long x)
> +{
> +  return x < 123UL ? -1UL : 0;
> +}
> +
> +unsigned long
> +f2 (unsigned int x)
> +{
> +  return x < 12345U ? -1UL : 0;
> +}
> +
> +unsigned long
> +f3 (unsigned short *x)
> +{
> +  return x[0] < 1234U ? -1UL : 0;
> +}
> +
> +unsigned long
> +f4 (unsigned char *x)
> +{
> +  return x[0] < 123U ? -1UL : 0;
> +}
> +
> +unsigned int
> +f5 (unsigned long x)
> +{
> +  return x < 123UL ? -1U : 0;
> +}
> +
> +unsigned int
> +f6 (unsigned int x)
> +{
> +  return x < 12345U ? -1U : 0;
> +}
> +
> +unsigned int
> +f7 (unsigned short *x)
> +{
> +  return x[0] < 1234U ? -1U : 0;
> +}
> +
> +unsigned int
> +f8 (unsigned char *x)
> +{
> +  return x[0] < 123U ? -1U : 0;
> +}
>
>         Jakub
Jakub Jelinek Dec. 11, 2018, 7:54 a.m. UTC | #2 
On Tue, Dec 11, 2018 at 08:44:00AM +0100, Uros Bizjak wrote:
> > --- gcc/config/i386/i386.md.jj  2018-11-22 10:40:31.179683319 +0100
> > +++ gcc/config/i386/i386.md     2018-12-10 11:24:49.785830186 +0100
> > @@ -17195,6 +17195,24 @@ (define_insn "*x86_mov<mode>cc_0_m1_neg"
> >     (set_attr "mode" "<MODE>")
> >     (set_attr "length_immediate" "0")])
> >
> > +(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
> > +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> > +       (neg:SWI48
> > +         (leu:SWI48
> > +           (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
> > +           (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))
> 
> You can use const_int_operand predicate with "n" constraint here.

The point was to disallow 64-bit constants, so if I use const_int_operand
above, I'd need to replace CONST_INT_P (operands[2]) test with
trunc_int_for_mode (INTVAL (operands[2]), SImode) == INTVAL (operands[2])
or similar, perhaps do it for the 64-bit mode only, so
  (<SWI:MODE>mode != DImode
   || (trunc_int_for_mode (INTVAL (operands[2]), SImode)
       == INTVAL (operands[2])))

> > +   (clobber (reg:CC FLAGS_REG))]
> > +  "CONST_INT_P (operands[2])
> > +   && INTVAL (operands[2]) != -1
> > +   && INTVAL (operands[2]) != 2147483647"
> 
> Can UINTVAL be used here?

Just for the latter, or for both?  For the first one it would
require UINTVAL (operands[2]) != HOST_WIDE_INT_M1U
or so.

	Jakub
Uros Bizjak Dec. 11, 2018, 8:03 a.m. UTC | #3 
On Tue, Dec 11, 2018 at 8:54 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Dec 11, 2018 at 08:44:00AM +0100, Uros Bizjak wrote:
> > > --- gcc/config/i386/i386.md.jj  2018-11-22 10:40:31.179683319 +0100
> > > +++ gcc/config/i386/i386.md     2018-12-10 11:24:49.785830186 +0100
> > > @@ -17195,6 +17195,24 @@ (define_insn "*x86_mov<mode>cc_0_m1_neg"
> > >     (set_attr "mode" "<MODE>")
> > >     (set_attr "length_immediate" "0")])
> > >
> > > +(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
> > > +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> > > +       (neg:SWI48
> > > +         (leu:SWI48
> > > +           (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
> > > +           (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))
> >
> > You can use const_int_operand predicate with "n" constraint here.
>
> The point was to disallow 64-bit constants, so if I use const_int_operand
> above, I'd need to replace CONST_INT_P (operands[2]) test with
> trunc_int_for_mode (INTVAL (operands[2]), SImode) == INTVAL (operands[2])
> or similar, perhaps do it for the 64-bit mode only, so
>   (<SWI:MODE>mode != DImode
>    || (trunc_int_for_mode (INTVAL (operands[2]), SImode)
>        == INTVAL (operands[2])))

The above is the preferred way (we already have a couple of instances
in predicates, where trunc_int_for_mode is used for the above case).
I'd recommend to put everything in the preparation statement and FAIL
in case the value is not supported.
Like:

{
  HOST_WIDE_INT val = UINTVAL operands[2];
  if (trunc_int_for_mode (val, SImode) != val || val != -1)
  FAIL;
 ...
}

Uros.

> > > +   (clobber (reg:CC FLAGS_REG))]
> > > +  "CONST_INT_P (operands[2])
> > > +   && INTVAL (operands[2]) != -1
> > > +   && INTVAL (operands[2]) != 2147483647"
> >
> > Can UINTVAL be used here?
>
> Just for the latter, or for both?  For the first one it would
> require UINTVAL (operands[2]) != HOST_WIDE_INT_M1U
> or so.
>
>         Jakub
Jakub Jelinek Dec. 11, 2018, 8:14 a.m. UTC | #4 
On Tue, Dec 11, 2018 at 09:03:08AM +0100, Uros Bizjak wrote:
> On Tue, Dec 11, 2018 at 8:54 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Tue, Dec 11, 2018 at 08:44:00AM +0100, Uros Bizjak wrote:
> > > > --- gcc/config/i386/i386.md.jj  2018-11-22 10:40:31.179683319 +0100
> > > > +++ gcc/config/i386/i386.md     2018-12-10 11:24:49.785830186 +0100
> > > > @@ -17195,6 +17195,24 @@ (define_insn "*x86_mov<mode>cc_0_m1_neg"
> > > >     (set_attr "mode" "<MODE>")
> > > >     (set_attr "length_immediate" "0")])
> > > >
> > > > +(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
> > > > +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> > > > +       (neg:SWI48
> > > > +         (leu:SWI48
> > > > +           (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
> > > > +           (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))
> > >
> > > You can use const_int_operand predicate with "n" constraint here.
> >
> > The point was to disallow 64-bit constants, so if I use const_int_operand
> > above, I'd need to replace CONST_INT_P (operands[2]) test with
> > trunc_int_for_mode (INTVAL (operands[2]), SImode) == INTVAL (operands[2])
> > or similar, perhaps do it for the 64-bit mode only, so
> >   (<SWI:MODE>mode != DImode
> >    || (trunc_int_for_mode (INTVAL (operands[2]), SImode)
> >        == INTVAL (operands[2])))
> 
> The above is the preferred way (we already have a couple of instances
> in predicates, where trunc_int_for_mode is used for the above case).
> I'd recommend to put everything in the preparation statement and FAIL
> in case the value is not supported.
> Like:
> 
> {
>   HOST_WIDE_INT val = UINTVAL operands[2];
>   if (trunc_int_for_mode (val, SImode) != val || val != -1)
>   FAIL;
>  ...
> }

If it is in preparation statements, then if it does FAIL, then it will ICE,
because it matched as insn already, the FAIL would just mean it couldn't be
split and we then would need to provide some pattern to handle it.

	Jakub
Uros Bizjak Dec. 11, 2018, 9:07 a.m. UTC | #5 
On Tue, Dec 11, 2018 at 9:14 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Dec 11, 2018 at 09:03:08AM +0100, Uros Bizjak wrote:
> > On Tue, Dec 11, 2018 at 8:54 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > On Tue, Dec 11, 2018 at 08:44:00AM +0100, Uros Bizjak wrote:
> > > > > --- gcc/config/i386/i386.md.jj  2018-11-22 10:40:31.179683319 +0100
> > > > > +++ gcc/config/i386/i386.md     2018-12-10 11:24:49.785830186 +0100
> > > > > @@ -17195,6 +17195,24 @@ (define_insn "*x86_mov<mode>cc_0_m1_neg"
> > > > >     (set_attr "mode" "<MODE>")
> > > > >     (set_attr "length_immediate" "0")])
> > > > >
> > > > > +(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
> > > > > +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> > > > > +       (neg:SWI48
> > > > > +         (leu:SWI48
> > > > > +           (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
> > > > > +           (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))
> > > >
> > > > You can use const_int_operand predicate with "n" constraint here.
> > >
> > > The point was to disallow 64-bit constants, so if I use const_int_operand
> > > above, I'd need to replace CONST_INT_P (operands[2]) test with
> > > trunc_int_for_mode (INTVAL (operands[2]), SImode) == INTVAL (operands[2])
> > > or similar, perhaps do it for the 64-bit mode only, so
> > >   (<SWI:MODE>mode != DImode
> > >    || (trunc_int_for_mode (INTVAL (operands[2]), SImode)
> > >        == INTVAL (operands[2])))
> >
> > The above is the preferred way (we already have a couple of instances
> > in predicates, where trunc_int_for_mode is used for the above case).
> > I'd recommend to put everything in the preparation statement and FAIL
> > in case the value is not supported.
> > Like:
> >
> > {
> >   HOST_WIDE_INT val = UINTVAL operands[2];
> >   if (trunc_int_for_mode (val, SImode) != val || val != -1)
> >   FAIL;
> >  ...
> > }
>
> If it is in preparation statements, then if it does FAIL, then it will ICE,
> because it matched as insn already, the FAIL would just mean it couldn't be
> split and we then would need to provide some pattern to handle it.

OK, let's go with your original patch then. Other approaches are more
complex that the original patch.

Thanks,
Uros.
diff mbox series

Patch

--- gcc/config/i386/i386.md.jj	2018-11-22 10:40:31.179683319 +0100
+++ gcc/config/i386/i386.md	2018-12-10 11:24:49.785830186 +0100
@@ -17195,6 +17195,24 @@  (define_insn "*x86_mov<mode>cc_0_m1_neg"
    (set_attr "mode" "<MODE>")
    (set_attr "length_immediate" "0")])
 
+(define_insn_and_split "*x86_mov<SWI48:mode>cc_0_m1_neg_leu<SWI:mode>"
+  [(set (match_operand:SWI48 0 "register_operand" "=r")
+	(neg:SWI48
+	  (leu:SWI48
+	    (match_operand:SWI 1 "nonimmediate_operand" "<SWI:r>m")
+	    (match_operand:SWI 2 "<SWI:immediate_operand>" "<SWI:i>"))))
+   (clobber (reg:CC FLAGS_REG))]
+  "CONST_INT_P (operands[2])
+   && INTVAL (operands[2]) != -1
+   && INTVAL (operands[2]) != 2147483647"
+  "#"
+  ""
+  [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2)))
+   (parallel [(set (match_dup 0)
+		   (neg:SWI48 (ltu:SWI48 (reg:CC FLAGS_REG) (const_int 0))))
+	      (clobber (reg:CC FLAGS_REG))])]
+  "operands[2] = GEN_INT (INTVAL (operands[2]) + 1);")
+
 (define_insn "*mov<mode>cc_noc"
   [(set (match_operand:SWI248 0 "register_operand" "=r,r")
 	(if_then_else:SWI248 (match_operator 1 "ix86_comparison_operator"
--- gcc/testsuite/gcc.target/i386/pr88425.c.jj	2018-12-10 11:31:35.507196453 +0100
+++ gcc/testsuite/gcc.target/i386/pr88425.c	2018-12-10 11:31:17.231495274 +0100
@@ -0,0 +1,53 @@ 
+/* PR target/88425 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -masm=att" } */
+/* { dg-final { scan-assembler-times "sbb\[lq]\[ \t]" 8 } } */
+/* { dg-final { scan-assembler-not "setbe\[ \t]" } } */
+
+unsigned long
+f1 (unsigned long x)
+{
+  return x < 123UL ? -1UL : 0;
+}
+
+unsigned long
+f2 (unsigned int x)
+{
+  return x < 12345U ? -1UL : 0;
+}
+
+unsigned long
+f3 (unsigned short *x)
+{
+  return x[0] < 1234U ? -1UL : 0;
+}
+
+unsigned long
+f4 (unsigned char *x)
+{
+  return x[0] < 123U ? -1UL : 0;
+}
+
+unsigned int
+f5 (unsigned long x)
+{
+  return x < 123UL ? -1U : 0;
+}
+
+unsigned int
+f6 (unsigned int x)
+{
+  return x < 12345U ? -1U : 0;
+}
+
+unsigned int
+f7 (unsigned short *x)
+{
+  return x[0] < 1234U ? -1U : 0;
+}
+
+unsigned int
+f8 (unsigned char *x)
+{
+  return x[0] < 123U ? -1U : 0;
+}