| Message ID | CAAkRFZKK33rdMVLGyB8S0iLSm4k5Qosug0phieV2rXv8tmgMnA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
With gcc regression test, no regressions are found. David On Tue, Sep 9, 2014 at 11:45 AM, Xinliang David Li <davidxl@google.com> wrote: > Richard, thanks for the review. The revised patch is attached. Is this > one OK (after testing is done)? > > David > > > 2014-09-08 Xinliang David Li <davidxl@google.com> > > PR target/63209 > * config/arm/arm.md (movcond_addsi): Handle case where source > and target operands are the same > > 2014-09-08 Xinliang David Li <davidxl@google.com> > > PR target/63209 > * gcc.c-torture/execute/pr63209.c: New test > > > > On Tue, Sep 9, 2014 at 10:31 AM, Richard Earnshaw <rearnsha@arm.com> wrote: >> On 09/09/14 16:58, Xinliang David Li wrote: >>> The attached patch fixed the problem reported in PR63209. The problem >>> exists in both gcc4.9 and trunk. >>> >>> Regression test on arm-unknown-linux-gnueabi passes. >>> >>> Ok for gcc-4.9 and trunk? >>> >> >> No, this isn't right. I don't think you need a new pattern here (you >> certainly don't want a new insn - at most you would just need a new >> split). But I think you just need some special case code in the >> existing pattern to handle the overlap case. >> >> Also, please do not post ChangeLog entries in patch format; they won't >> apply by the time the patch is ready to be committed. >> >> R. >> >> >>> (I sent the patch last night, but it got lost somehow) >>> >>> >>> David >>> >>> >>> pr63209.txt >>> >>> >>> Index: ChangeLog >>> =================================================================== >>> --- ChangeLog (revision 215039) >>> +++ ChangeLog (working copy) >>> @@ -1,3 +1,9 @@ >>> +2014-09-08 Xinliang David Li <davidxl@google.com> >>> + >>> + PR target/63209 >>> + * config/arm/arm.md (movcond_addsi_1): New pattern. >>> + (movcond_addsi): Add a constraint. >>> + >>> 2014-09-08 Trevor Saunders <tsaunders@mozilla.com> >>> >>> * common/config/picochip/picochip-common.c: Remove. >>> Index: config/arm/arm.md >>> =================================================================== >>> --- config/arm/arm.md (revision 215039) >>> +++ config/arm/arm.md (working copy) >>> @@ -9302,6 +9302,43 @@ >>> (set_attr "type" "multiple")] >>> ) >>> >>> +(define_insn_and_split "movcond_addsi_1" >>> + [(set (match_operand:SI 0 "s_register_operand" "=r,l,r") >>> + (if_then_else:SI >>> + (match_operator 4 "comparison_operator" >>> + [(plus:SI (match_operand:SI 2 "s_register_operand" "r,r,r") >>> + (match_operand:SI 3 "arm_add_operand" "rIL,rIL,rIL")) >>> + (const_int 0)]) >>> + (match_operand:SI 1 "arm_rhs_operand" "rI,rPy,r") >>> + (match_dup:SI 0))) >>> + (clobber (reg:CC CC_REGNUM))] >>> + "TARGET_32BIT" >>> + "#" >>> + "&& reload_completed" >>> + [(set (reg:CC_NOOV CC_REGNUM) >>> + (compare:CC_NOOV >>> + (plus:SI (match_dup 2) >>> + (match_dup 3)) >>> + (const_int 0))) >>> + (cond_exec (match_dup 5) >>> + (set (match_dup 0) (match_dup 1)))] >>> + " >>> + { >>> + enum machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[4]), >>> + operands[2], operands[3]); >>> + enum rtx_code rc = GET_CODE (operands[4]); >>> + >>> + operands[5] = gen_rtx_REG (mode, CC_REGNUM); >>> + gcc_assert (!(mode == CCFPmode || mode == CCFPEmode)); >>> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[5], const0_rtx); >>> + } >>> + " >>> + [(set_attr "conds" "clob") >>> + (set_attr "enabled_for_depr_it" "no,yes,yes") >>> + (set_attr "type" "multiple")] >>> +) >>> + >>> + >>> (define_insn_and_split "movcond_addsi" >>> [(set (match_operand:SI 0 "s_register_operand" "=r,l,r") >>> (if_then_else:SI >>> @@ -9312,7 +9349,7 @@ >>> (match_operand:SI 1 "arm_rhs_operand" "rI,rPy,r") >>> (match_operand:SI 2 "arm_rhs_operand" "rI,rPy,r"))) >>> (clobber (reg:CC CC_REGNUM))] >>> - "TARGET_32BIT" >>> + "TARGET_32BIT && REGNO (operands[2]) != REGNO (operands[0])" >>> "#" >>> "&& reload_completed" >>> [(set (reg:CC_NOOV CC_REGNUM) >>> Index: testsuite/ChangeLog >>> =================================================================== >>> --- testsuite/ChangeLog (revision 215039) >>> +++ testsuite/ChangeLog (working copy) >>> @@ -1,3 +1,8 @@ >>> +2014-09-08 Xinliang David Li <davidxl@google.com> >>> + >>> + PR target/63209 >>> + * gcc.c-torture/execute/pr63209.c: New test >>> + >>> 2014-09-08 Jakub Jelinek <jakub@redhat.com> >>> >>> PR tree-optimization/60196 >>> Index: testsuite/gcc.c-torture/execute/pr63209.c >>> =================================================================== >>> --- testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >>> +++ testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >>> @@ -0,0 +1,27 @@ >>> +static int Sub(int a, int b) { >>> + return b -a; >>> +} >>> + >>> +static unsigned Select(unsigned a, unsigned b, unsigned c) { >>> + const int pa_minus_pb = >>> + Sub((a >> 8) & 0xff, (b >> 8) & 0xff) + >>> + Sub((a >> 0) & 0xff, (b >> 0) & 0xff); >>> + return (pa_minus_pb <= 0) ? a : b; >>> +} >>> + >>> +__attribute__((noinline)) unsigned Predictor(unsigned left, const unsigned* const top) { >>> + const unsigned pred = Select(top[1], left, top[0]); >>> + return pred; >>> +} >>> + >>> +int main(void) { >>> + const unsigned top[2] = {0xff7a7a7a, 0xff7a7a7a}; >>> + const unsigned left = 0xff7b7b7b; >>> + const unsigned pred = Predictor(left, top /*+ 1*/); >>> + if (pred == left) >>> + return 0; >>> + return 1; >>> +} >>> + >>> + >>> + >>> >> >>
On 09/09/14 19:45, Xinliang David Li wrote: > Richard, thanks for the review. The revised patch is attached. Is this > one OK (after testing is done)? > > David > > OK, but ... > 2014-09-08 Xinliang David Li <davidxl@google.com> > > PR target/63209 > * config/arm/arm.md (movcond_addsi): Handle case where source > and target operands are the same Full stop at end of sentence. > > 2014-09-08 Xinliang David Li <davidxl@google.com> > > PR target/63209 > * gcc.c-torture/execute/pr63209.c: New test > Likewise. > pr63209.txt > > > Index: config/arm/arm.md > =================================================================== > --- config/arm/arm.md (revision 215039) > +++ config/arm/arm.md (working copy) > @@ -9328,10 +9328,16 @@ > enum machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[5]), > operands[3], operands[4]); > enum rtx_code rc = GET_CODE (operands[5]); > - > operands[6] = gen_rtx_REG (mode, CC_REGNUM); > gcc_assert (!(mode == CCFPmode || mode == CCFPEmode)); > - rc = reverse_condition (rc); > + if (REGNO (operands[2]) != REGNO (operands[0])) > + rc = reverse_condition (rc); > + else > + { > + rtx tmp = operands[1]; > + operands[1] = operands[2]; > + operands[2] = tmp; Please use tabs and white space consistently (use tabs). Also, you seem to have some trailing white space at the end of some lines. > + } > > operands[6] = gen_rtx_fmt_ee (rc, VOIDmode, operands[6], const0_rtx); > } > Index: testsuite/gcc.c-torture/execute/pr63209.c > =================================================================== > --- testsuite/gcc.c-torture/execute/pr63209.c (revision 0) > +++ testsuite/gcc.c-torture/execute/pr63209.c (revision 0) > @@ -0,0 +1,27 @@ > +static int Sub(int a, int b) { > + return b -a; > +} > + > +static unsigned Select(unsigned a, unsigned b, unsigned c) { > + const int pa_minus_pb = > + Sub((a >> 8) & 0xff, (b >> 8) & 0xff) + > + Sub((a >> 0) & 0xff, (b >> 0) & 0xff); > + return (pa_minus_pb <= 0) ? a : b; > +} > + > +__attribute__((noinline)) unsigned Predictor(unsigned left, const unsigned* const top) { > + const unsigned pred = Select(top[1], left, top[0]); > + return pred; > +} > + > +int main(void) { > + const unsigned top[2] = {0xff7a7a7a, 0xff7a7a7a}; > + const unsigned left = 0xff7b7b7b; > + const unsigned pred = Predictor(left, top /*+ 1*/); > + if (pred == left) > + return 0; > + return 1; > +} > + > + > + >
Fixed the formatting and committed it to trunk: r215136. Will backport it to gcc-4_9 branch. thanks, David On Wed, Sep 10, 2014 at 11:24 AM, Richard Earnshaw <rearnsha@arm.com> wrote: > On 09/09/14 19:45, Xinliang David Li wrote: >> Richard, thanks for the review. The revised patch is attached. Is this >> one OK (after testing is done)? >> >> David >> >> > > OK, but ... > >> 2014-09-08 Xinliang David Li <davidxl@google.com> >> >> PR target/63209 >> * config/arm/arm.md (movcond_addsi): Handle case where source >> and target operands are the same > > Full stop at end of sentence. > >> >> 2014-09-08 Xinliang David Li <davidxl@google.com> >> >> PR target/63209 >> * gcc.c-torture/execute/pr63209.c: New test >> > > Likewise. > >> pr63209.txt >> >> >> Index: config/arm/arm.md >> =================================================================== >> --- config/arm/arm.md (revision 215039) >> +++ config/arm/arm.md (working copy) >> @@ -9328,10 +9328,16 @@ >> enum machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[5]), >> operands[3], operands[4]); >> enum rtx_code rc = GET_CODE (operands[5]); >> - >> operands[6] = gen_rtx_REG (mode, CC_REGNUM); >> gcc_assert (!(mode == CCFPmode || mode == CCFPEmode)); >> - rc = reverse_condition (rc); >> + if (REGNO (operands[2]) != REGNO (operands[0])) >> + rc = reverse_condition (rc); >> + else >> + { >> + rtx tmp = operands[1]; >> + operands[1] = operands[2]; >> + operands[2] = tmp; > > Please use tabs and white space consistently (use tabs). Also, you seem > to have some trailing white space at the end of some lines. > >> + } >> >> operands[6] = gen_rtx_fmt_ee (rc, VOIDmode, operands[6], const0_rtx); >> } >> Index: testsuite/gcc.c-torture/execute/pr63209.c >> =================================================================== >> --- testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >> +++ testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >> @@ -0,0 +1,27 @@ >> +static int Sub(int a, int b) { >> + return b -a; >> +} >> + >> +static unsigned Select(unsigned a, unsigned b, unsigned c) { >> + const int pa_minus_pb = >> + Sub((a >> 8) & 0xff, (b >> 8) & 0xff) + >> + Sub((a >> 0) & 0xff, (b >> 0) & 0xff); >> + return (pa_minus_pb <= 0) ? a : b; >> +} >> + >> +__attribute__((noinline)) unsigned Predictor(unsigned left, const unsigned* const top) { >> + const unsigned pred = Select(top[1], left, top[0]); >> + return pred; >> +} >> + >> +int main(void) { >> + const unsigned top[2] = {0xff7a7a7a, 0xff7a7a7a}; >> + const unsigned left = 0xff7b7b7b; >> + const unsigned pred = Predictor(left, top /*+ 1*/); >> + if (pred == left) >> + return 0; >> + return 1; >> +} >> + >> + >> + >> > >
Index: config/arm/arm.md =================================================================== --- config/arm/arm.md (revision 215039) +++ config/arm/arm.md (working copy) @@ -9328,10 +9328,16 @@ enum machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[5]), operands[3], operands[4]); enum rtx_code rc = GET_CODE (operands[5]); - operands[6] = gen_rtx_REG (mode, CC_REGNUM); gcc_assert (!(mode == CCFPmode || mode == CCFPEmode)); - rc = reverse_condition (rc); + if (REGNO (operands[2]) != REGNO (operands[0])) + rc = reverse_condition (rc); + else + { + rtx tmp = operands[1]; + operands[1] = operands[2]; + operands[2] = tmp; + } operands[6] = gen_rtx_fmt_ee (rc, VOIDmode, operands[6], const0_rtx); } Index: testsuite/gcc.c-torture/execute/pr63209.c =================================================================== --- testsuite/gcc.c-torture/execute/pr63209.c (revision 0) +++ testsuite/gcc.c-torture/execute/pr63209.c (revision 0) @@ -0,0 +1,27 @@ +static int Sub(int a, int b) { + return b -a; +} + +static unsigned Select(unsigned a, unsigned b, unsigned c) { + const int pa_minus_pb = + Sub((a >> 8) & 0xff, (b >> 8) & 0xff) + + Sub((a >> 0) & 0xff, (b >> 0) & 0xff); + return (pa_minus_pb <= 0) ? a : b; +} + +__attribute__((noinline)) unsigned Predictor(unsigned left, const unsigned* const top) { + const unsigned pred = Select(top[1], left, top[0]); + return pred; +} + +int main(void) { + const unsigned top[2] = {0xff7a7a7a, 0xff7a7a7a}; + const unsigned left = 0xff7b7b7b; + const unsigned pred = Predictor(left, top /*+ 1*/); + if (pred == left) + return 0; + return 1; +} + + +
Richard, thanks for the review. The revised patch is attached. Is this one OK (after testing is done)? David 2014-09-08 Xinliang David Li <davidxl@google.com> PR target/63209 * config/arm/arm.md (movcond_addsi): Handle case where source and target operands are the same 2014-09-08 Xinliang David Li <davidxl@google.com> PR target/63209 * gcc.c-torture/execute/pr63209.c: New test On Tue, Sep 9, 2014 at 10:31 AM, Richard Earnshaw <rearnsha@arm.com> wrote: > On 09/09/14 16:58, Xinliang David Li wrote: >> The attached patch fixed the problem reported in PR63209. The problem >> exists in both gcc4.9 and trunk. >> >> Regression test on arm-unknown-linux-gnueabi passes. >> >> Ok for gcc-4.9 and trunk? >> > > No, this isn't right. I don't think you need a new pattern here (you > certainly don't want a new insn - at most you would just need a new > split). But I think you just need some special case code in the > existing pattern to handle the overlap case. > > Also, please do not post ChangeLog entries in patch format; they won't > apply by the time the patch is ready to be committed. > > R. > > >> (I sent the patch last night, but it got lost somehow) >> >> >> David >> >> >> pr63209.txt >> >> >> Index: ChangeLog >> =================================================================== >> --- ChangeLog (revision 215039) >> +++ ChangeLog (working copy) >> @@ -1,3 +1,9 @@ >> +2014-09-08 Xinliang David Li <davidxl@google.com> >> + >> + PR target/63209 >> + * config/arm/arm.md (movcond_addsi_1): New pattern. >> + (movcond_addsi): Add a constraint. >> + >> 2014-09-08 Trevor Saunders <tsaunders@mozilla.com> >> >> * common/config/picochip/picochip-common.c: Remove. >> Index: config/arm/arm.md >> =================================================================== >> --- config/arm/arm.md (revision 215039) >> +++ config/arm/arm.md (working copy) >> @@ -9302,6 +9302,43 @@ >> (set_attr "type" "multiple")] >> ) >> >> +(define_insn_and_split "movcond_addsi_1" >> + [(set (match_operand:SI 0 "s_register_operand" "=r,l,r") >> + (if_then_else:SI >> + (match_operator 4 "comparison_operator" >> + [(plus:SI (match_operand:SI 2 "s_register_operand" "r,r,r") >> + (match_operand:SI 3 "arm_add_operand" "rIL,rIL,rIL")) >> + (const_int 0)]) >> + (match_operand:SI 1 "arm_rhs_operand" "rI,rPy,r") >> + (match_dup:SI 0))) >> + (clobber (reg:CC CC_REGNUM))] >> + "TARGET_32BIT" >> + "#" >> + "&& reload_completed" >> + [(set (reg:CC_NOOV CC_REGNUM) >> + (compare:CC_NOOV >> + (plus:SI (match_dup 2) >> + (match_dup 3)) >> + (const_int 0))) >> + (cond_exec (match_dup 5) >> + (set (match_dup 0) (match_dup 1)))] >> + " >> + { >> + enum machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[4]), >> + operands[2], operands[3]); >> + enum rtx_code rc = GET_CODE (operands[4]); >> + >> + operands[5] = gen_rtx_REG (mode, CC_REGNUM); >> + gcc_assert (!(mode == CCFPmode || mode == CCFPEmode)); >> + operands[5] = gen_rtx_fmt_ee (rc, VOIDmode, operands[5], const0_rtx); >> + } >> + " >> + [(set_attr "conds" "clob") >> + (set_attr "enabled_for_depr_it" "no,yes,yes") >> + (set_attr "type" "multiple")] >> +) >> + >> + >> (define_insn_and_split "movcond_addsi" >> [(set (match_operand:SI 0 "s_register_operand" "=r,l,r") >> (if_then_else:SI >> @@ -9312,7 +9349,7 @@ >> (match_operand:SI 1 "arm_rhs_operand" "rI,rPy,r") >> (match_operand:SI 2 "arm_rhs_operand" "rI,rPy,r"))) >> (clobber (reg:CC CC_REGNUM))] >> - "TARGET_32BIT" >> + "TARGET_32BIT && REGNO (operands[2]) != REGNO (operands[0])" >> "#" >> "&& reload_completed" >> [(set (reg:CC_NOOV CC_REGNUM) >> Index: testsuite/ChangeLog >> =================================================================== >> --- testsuite/ChangeLog (revision 215039) >> +++ testsuite/ChangeLog (working copy) >> @@ -1,3 +1,8 @@ >> +2014-09-08 Xinliang David Li <davidxl@google.com> >> + >> + PR target/63209 >> + * gcc.c-torture/execute/pr63209.c: New test >> + >> 2014-09-08 Jakub Jelinek <jakub@redhat.com> >> >> PR tree-optimization/60196 >> Index: testsuite/gcc.c-torture/execute/pr63209.c >> =================================================================== >> --- testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >> +++ testsuite/gcc.c-torture/execute/pr63209.c (revision 0) >> @@ -0,0 +1,27 @@ >> +static int Sub(int a, int b) { >> + return b -a; >> +} >> + >> +static unsigned Select(unsigned a, unsigned b, unsigned c) { >> + const int pa_minus_pb = >> + Sub((a >> 8) & 0xff, (b >> 8) & 0xff) + >> + Sub((a >> 0) & 0xff, (b >> 0) & 0xff); >> + return (pa_minus_pb <= 0) ? a : b; >> +} >> + >> +__attribute__((noinline)) unsigned Predictor(unsigned left, const unsigned* const top) { >> + const unsigned pred = Select(top[1], left, top[0]); >> + return pred; >> +} >> + >> +int main(void) { >> + const unsigned top[2] = {0xff7a7a7a, 0xff7a7a7a}; >> + const unsigned left = 0xff7b7b7b; >> + const unsigned pred = Predictor(left, top /*+ 1*/); >> + if (pred == left) >> + return 0; >> + return 1; >> +} >> + >> + >> + >> > >