| Message ID | 20180327211159.GE8577@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Improve adc discovery during combine on x86 (PR target/85095) | expand |
On Tue, Mar 27, 2018 at 11:11 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > In 6.x we've changed unsigned if (a < b) a++; into ADD_OVERFLOW ifn, > which results in different expanded code, which on the following testcase > unfortunately doesn't combine anymore into the optimal 3 instructions. > > The problem is that we want adc[lq] $0, %reg instruction, but simplify-rtx.c > leaves the apparently useless (plus something const0_rtx) out, just uses > something, and there is no pattern that matches that. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? > > 2018-03-27 Jakub Jelinek <jakub@redhat.com> > > PR target/85095 > * config/i386/i386.md (*add<mode>3_carry0): New pattern. > > * gcc.target/i386/pr85095.c: New test. > > --- gcc/config/i386/i386.md.jj 2018-03-27 12:54:54.685244368 +0200 > +++ gcc/config/i386/i386.md 2018-03-27 19:38:43.891451026 +0200 > @@ -6854,6 +6854,23 @@ (define_insn "add<mode>3_carry" > (set_attr "pent_pair" "pu") > (set_attr "mode" "<MODE>")]) > > +(define_insn "*add<mode>3_carry0" Please name this "*add<mode>3_carry_0". You will also need to introduce "*addsi3_carry_zext_0". Probably minus patterns have the same problem, simplify-rtx probably removes (minus ... const_rtx0), too. > + [(set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m") > + (plus:SWI > + (match_operator:SWI 3 "ix86_carry_flag_operator" > + [(match_operand 2 "flags_reg_operand") (const_int 0)]) > + (match_operand:SWI 1 "nonimmediate_operand" "0"))) > + (clobber (reg:CC FLAGS_REG))] > + "ix86_unary_operator_ok (PLUS, <MODE>mode, operands)" > +{ > + operands[4] = const0_rtx; > + return "adc{<imodesuffix>}\t{%4, %0|%0, %4}"; Just use "$0" ("0" in intel syntax) in the insn template. Uros.
On Wed, Mar 28, 2018 at 2:54 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 28, 2018 at 08:51:07AM +0200, Uros Bizjak wrote: >> > --- gcc/config/i386/i386.md.jj 2018-03-27 12:54:54.685244368 +0200 >> > +++ gcc/config/i386/i386.md 2018-03-27 19:38:43.891451026 +0200 >> > @@ -6854,6 +6854,23 @@ (define_insn "add<mode>3_carry" >> > (set_attr "pent_pair" "pu") >> > (set_attr "mode" "<MODE>")]) >> > >> > +(define_insn "*add<mode>3_carry0" >> >> Please name this "*add<mode>3_carry_0". You will also need to >> introduce "*addsi3_carry_zext_0". Probably minus patterns have the >> same problem, simplify-rtx probably removes (minus ... const_rtx0), >> too. >> >> > + [(set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m") >> > + (plus:SWI >> > + (match_operator:SWI 3 "ix86_carry_flag_operator" >> > + [(match_operand 2 "flags_reg_operand") (const_int 0)]) >> > + (match_operand:SWI 1 "nonimmediate_operand" "0"))) >> > + (clobber (reg:CC FLAGS_REG))] >> > + "ix86_unary_operator_ok (PLUS, <MODE>mode, operands)" >> > +{ >> > + operands[4] = const0_rtx; >> > + return "adc{<imodesuffix>}\t{%4, %0|%0, %4}"; >> >> Just use "$0" ("0" in intel syntax) in the insn template. > > So, like this, if it passes bootstrap/regtest? The testcases use all > 4 new patterns now. Yes, this is OK for mainline and eventual backports to release branches. Thanks, Uros. > 2018-03-28 Jakub Jelinek <jakub@redhat.com> > > PR target/85095 > * config/i386/i386.md (*add<mode>3_carry_0, *addsi3_carry_zext_0, > *sub<mode>3_carry_0, *subsi3_carry_zext_0): New patterns. > > * gcc.target/i386/pr85095-1.c: New test. > * gcc.target/i386/pr85095-2.c: New test. > * gcc.c-torture/execute/pr85095.c: New test. > > --- gcc/config/i386/i386.md.jj 2018-03-27 21:56:42.855454830 +0200 > +++ gcc/config/i386/i386.md 2018-03-28 14:12:35.430079720 +0200 > @@ -6854,6 +6854,20 @@ (define_insn "add<mode>3_carry" > (set_attr "pent_pair" "pu") > (set_attr "mode" "<MODE>")]) > > +(define_insn "*add<mode>3_carry_0" > + [(set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m") > + (plus:SWI > + (match_operator:SWI 3 "ix86_carry_flag_operator" > + [(match_operand 2 "flags_reg_operand") (const_int 0)]) > + (match_operand:SWI 1 "nonimmediate_operand" "0"))) > + (clobber (reg:CC FLAGS_REG))] > + "ix86_unary_operator_ok (PLUS, <MODE>mode, operands)" > + "adc{<imodesuffix>}\t{$0, %0|%0, 0}" > + [(set_attr "type" "alu") > + (set_attr "use_carry" "1") > + (set_attr "pent_pair" "pu") > + (set_attr "mode" "<MODE>")]) > + > (define_insn "*addsi3_carry_zext" > [(set (match_operand:DI 0 "register_operand" "=r") > (zero_extend:DI > @@ -6870,6 +6884,20 @@ (define_insn "*addsi3_carry_zext" > (set_attr "pent_pair" "pu") > (set_attr "mode" "SI")]) > > +(define_insn "*addsi3_carry_zext_0" > + [(set (match_operand:DI 0 "register_operand" "=r") > + (zero_extend:DI > + (plus:SI (match_operator:SI 2 "ix86_carry_flag_operator" > + [(reg FLAGS_REG) (const_int 0)]) > + (match_operand:SI 1 "register_operand" "0")))) > + (clobber (reg:CC FLAGS_REG))] > + "TARGET_64BIT" > + "adc{l}\t{$0, %k0|%k0, 0}" > + [(set_attr "type" "alu") > + (set_attr "use_carry" "1") > + (set_attr "pent_pair" "pu") > + (set_attr "mode" "SI")]) > + > ;; There is no point to generate ADCX instruction. ADC is shorter and faster. > > (define_insn "addcarry<mode>" > @@ -6926,6 +6954,20 @@ (define_insn "sub<mode>3_carry" > (set_attr "pent_pair" "pu") > (set_attr "mode" "<MODE>")]) > > +(define_insn "*sub<mode>3_carry_0" > + [(set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m") > + (minus:SWI > + (match_operand:SWI 1 "nonimmediate_operand" "0") > + (match_operator:SWI 3 "ix86_carry_flag_operator" > + [(match_operand 2 "flags_reg_operand") (const_int 0)]))) > + (clobber (reg:CC FLAGS_REG))] > + "ix86_unary_operator_ok (MINUS, <MODE>mode, operands)" > + "sbb{<imodesuffix>}\t{$0, %0|%0, 0}" > + [(set_attr "type" "alu") > + (set_attr "use_carry" "1") > + (set_attr "pent_pair" "pu") > + (set_attr "mode" "<MODE>")]) > + > (define_insn "*subsi3_carry_zext" > [(set (match_operand:DI 0 "register_operand" "=r") > (zero_extend:DI > @@ -6941,6 +6983,21 @@ (define_insn "*subsi3_carry_zext" > [(set_attr "type" "alu") > (set_attr "use_carry" "1") > (set_attr "pent_pair" "pu") > + (set_attr "mode" "SI")]) > + > +(define_insn "*subsi3_carry_zext_0" > + [(set (match_operand:DI 0 "register_operand" "=r") > + (zero_extend:DI > + (minus:SI > + (match_operand:SI 1 "register_operand" "0") > + (match_operator:SI 2 "ix86_carry_flag_operator" > + [(reg FLAGS_REG) (const_int 0)])))) > + (clobber (reg:CC FLAGS_REG))] > + "TARGET_64BIT" > + "sbb{l}\t{$0, %k0|%k0, 0}" > + [(set_attr "type" "alu") > + (set_attr "use_carry" "1") > + (set_attr "pent_pair" "pu") > (set_attr "mode" "SI")]) > > (define_insn "sub<mode>3_carry_ccc" > --- gcc/testsuite/gcc.target/i386/pr85095-1.c.jj 2018-03-28 14:30:17.929554377 +0200 > +++ gcc/testsuite/gcc.target/i386/pr85095-1.c 2018-03-28 14:44:04.082960231 +0200 > @@ -0,0 +1,33 @@ > +/* PR target/85095 * > +/* { dg-do compile } */ > +/* { dg-options "-O2 -masm=att" } */ > + > +unsigned int > +foo (unsigned int a, unsigned int b) > +{ > + a += b; > + if (a < b) a++; > + return a; > +} > + > +#ifdef __x86_64__ > +unsigned long long > +bar (unsigned long long a, unsigned long long b) > +{ > + a += b; > + if (a < b) a++; > + return a; > +} > + > +unsigned long long > +baz (unsigned int a, unsigned int b) > +{ > + a += b; > + if (a < b) a++; > + return a; > +} > +#endif > + > +/* { dg-final { scan-assembler-times "adcl\t\\\$0," 1 { target ia32 } } } */ > +/* { dg-final { scan-assembler-times "adcl\t\\\$0," 2 { target { ! ia32 } } } } */ > +/* { dg-final { scan-assembler-times "adcq\t\\\$0," 1 { target { ! ia32 } } } } */ > --- gcc/testsuite/gcc.target/i386/pr85095-2.c.jj 2018-03-28 14:30:21.037555723 +0200 > +++ gcc/testsuite/gcc.target/i386/pr85095-2.c 2018-03-28 14:50:34.353191266 +0200 > @@ -0,0 +1,54 @@ > +/* PR target/85095 * > +/* { dg-do compile } */ > +/* { dg-options "-O2 -masm=att" } */ > + > +unsigned int > +f1 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a + i; > +} > + > +unsigned int > +f2 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a - i; > +} > + > +#ifdef __x86_64__ > +unsigned long long > +f3 (unsigned long long a, unsigned long long b) > +{ > + unsigned long long i = __builtin_add_overflow (a, b, &a); > + return a + i; > +} > + > +unsigned long long > +f4 (unsigned long long a, unsigned long long b) > +{ > + unsigned long long i = __builtin_add_overflow (a, b, &a); > + return a - i; > +} > + > +unsigned long long > +f5 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a + i; > +} > + > +unsigned long long > +f6 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a - i; > +} > +#endif > + > +/* { dg-final { scan-assembler-times "adcl\t\\\$0," 1 { target ia32 } } } */ > +/* { dg-final { scan-assembler-times "sbbl\t\\\$0," 1 { target ia32 } } } */ > +/* { dg-final { scan-assembler-times "adcl\t\\\$0," 2 { target { ! ia32 } } } } */ > +/* { dg-final { scan-assembler-times "sbbl\t\\\$0," 2 { target { ! ia32 } } } } */ > +/* { dg-final { scan-assembler-times "adcq\t\\\$0," 1 { target { ! ia32 } } } } */ > +/* { dg-final { scan-assembler-times "sbbq\t\\\$0," 1 { target { ! ia32 } } } } */ > --- gcc/testsuite/gcc.c-torture/execute/pr85095.c.jj 2018-03-28 14:32:03.353600010 +0200 > +++ gcc/testsuite/gcc.c-torture/execute/pr85095.c 2018-03-28 14:39:18.422791123 +0200 > @@ -0,0 +1,52 @@ > +/* PR target/85095 */ > + > +__attribute__((noipa)) unsigned long > +f1 (unsigned long a, unsigned long b) > +{ > + unsigned long i = __builtin_add_overflow (a, b, &a); > + return a + i; > +} > + > +__attribute__((noipa)) unsigned long > +f2 (unsigned long a, unsigned long b) > +{ > + unsigned long i = __builtin_add_overflow (a, b, &a); > + return a - i; > +} > + > +__attribute__((noipa)) unsigned long > +f3 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a + i; > +} > + > +__attribute__((noipa)) unsigned long > +f4 (unsigned int a, unsigned int b) > +{ > + unsigned int i = __builtin_add_overflow (a, b, &a); > + return a - i; > +} > + > +int > +main () > +{ > + if (f1 (16UL, -18UL) != -2UL > + || f1 (16UL, -17UL) != -1UL > + || f1 (16UL, -16UL) != 1UL > + || f1 (16UL, -15UL) != 2UL > + || f2 (24UL, -26UL) != -2UL > + || f2 (24UL, -25UL) != -1UL > + || f2 (24UL, -24UL) != -1UL > + || f2 (24UL, -23UL) != 0UL > + || f3 (32U, -34U) != -2U > + || f3 (32U, -33U) != -1U > + || f3 (32U, -32U) != 1U > + || f3 (32U, -31U) != 2U > + || f4 (35U, -37U) != -2U > + || f4 (35U, -36U) != -1U > + || f4 (35U, -35U) != -1U > + || f4 (35U, -34U) != 0U) > + __builtin_abort (); > + return 0; > +} > > > Jakub
--- gcc/config/i386/i386.md.jj 2018-03-27 12:54:54.685244368 +0200 +++ gcc/config/i386/i386.md 2018-03-27 19:38:43.891451026 +0200 @@ -6854,6 +6854,23 @@ (define_insn "add<mode>3_carry" (set_attr "pent_pair" "pu") (set_attr "mode" "<MODE>")]) +(define_insn "*add<mode>3_carry0" + [(set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m") + (plus:SWI + (match_operator:SWI 3 "ix86_carry_flag_operator" + [(match_operand 2 "flags_reg_operand") (const_int 0)]) + (match_operand:SWI 1 "nonimmediate_operand" "0"))) + (clobber (reg:CC FLAGS_REG))] + "ix86_unary_operator_ok (PLUS, <MODE>mode, operands)" +{ + operands[4] = const0_rtx; + return "adc{<imodesuffix>}\t{%4, %0|%0, %4}"; +} + [(set_attr "type" "alu") + (set_attr "use_carry" "1") + (set_attr "pent_pair" "pu") + (set_attr "mode" "<MODE>")]) + (define_insn "*addsi3_carry_zext" [(set (match_operand:DI 0 "register_operand" "=r") (zero_extend:DI --- gcc/testsuite/gcc.target/i386/pr85095.c.jj 2018-03-27 19:49:02.985677415 +0200 +++ gcc/testsuite/gcc.target/i386/pr85095.c 2018-03-27 19:49:28.076686590 +0200 @@ -0,0 +1,13 @@ +/* PR target/85095 * +/* { dg-do compile } */ +/* { dg-options "-O2 -masm=att" } */ + +unsigned long +foo (unsigned long a, unsigned long b) +{ + a += b; + if (a < b) a++; + return a; +} + +/* { dg-final { scan-assembler-times "adc\[lq]\t\\\$0," 1 } } */