| Message ID | CACgzC7DLadpmeQ9eS6Zn3jSJKF_LytV0hkjt3D8i8APMmhErzA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 23/06/14 07:58, Zhenqiang Chen wrote: > Hi, > > Swapping operands in a ccmp will lead to illegal instructions. So the > patch disables it in simplify_while_replacing. > > The patch is separated from > https://gcc.gnu.org/ml/gcc-patches/2014-02/msg01407.html. > > To make it clean. The patch adds two files: ccmp.{c,h} to hold all new > ccmp related functions. > > OK for trunk? > > Thanks! > -Zhenqiang > > ChangeLog: > 2014-06-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > * Makefile.in: Add ccmp.o > * ccmp.c: New file. Do we really need a new file for one 10-line function? Seems like overkill. I think it would be better to just drop this function into recog.c. Also, can you explain more clearly what the problem is with swapping the operands? If this can't be done, then SWAPPABLE_OPERANDS is arguably doing the wrong thing; and that might mean that rtx class you've applied to your new code is incorrect. R. > * ccmp.h: New file. > * recog.c (simplify_while_replacing): Check ccmp_insn_p. > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > index 5587b75..8757a30 100644 > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -1169,6 +1169,7 @@ OBJS = \ > builtins.o \ > caller-save.o \ > calls.o \ > + ccmp.o \ > cfg.o \ > cfganal.o \ > cfgbuild.o \ > diff --git a/gcc/ccmp.c b/gcc/ccmp.c > new file mode 100644 > index 0000000..665c2a5 > --- /dev/null > +++ b/gcc/ccmp.c > @@ -0,0 +1,62 @@ > +/* Conditional compare related functions > + Copyright (C) 2014-2014 Free Software Foundation, Inc. > + > +This file is part of GCC. > + > +GCC is free software; you can redistribute it and/or modify it under > +the terms of the GNU General Public License as published by the Free > +Software Foundation; either version 3, or (at your option) any later > +version. > + > +GCC is distributed in the hope that it will be useful, but WITHOUT ANY > +WARRANTY; without even the implied warranty of MERCHANTABILITY or > +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > +for more details. > + > +You should have received a copy of the GNU General Public License > +along with GCC; see the file COPYING3. If not see > +<http://www.gnu.org/licenses/>. */ > + > +#include "config.h" > +#include "system.h" > +#include "coretypes.h" > +#include "tm.h" > +#include "rtl.h" > +#include "tree.h" > +#include "stringpool.h" > +#include "regs.h" > +#include "expr.h" > +#include "optabs.h" > +#include "tree-iterator.h" > +#include "basic-block.h" > +#include "tree-ssa-alias.h" > +#include "internal-fn.h" > +#include "gimple-expr.h" > +#include "is-a.h" > +#include "gimple.h" > +#include "gimple-ssa.h" > +#include "tree-ssanames.h" > +#include "target.h" > +#include "common/common-target.h" > +#include "df.h" > +#include "tree-ssa-live.h" > +#include "tree-outof-ssa.h" > +#include "cfgexpand.h" > +#include "tree-phinodes.h" > +#include "ssa-iterators.h" > +#include "expmed.h" > +#include "ccmp.h" > + > +bool > +ccmp_insn_p (rtx object) > +{ > + rtx x = PATTERN (object); > + if (targetm.gen_ccmp_first > + && GET_CODE (x) == SET > + && GET_CODE (XEXP (x, 1)) == COMPARE > + && (GET_CODE (XEXP (XEXP (x, 1), 0)) == IOR > + || GET_CODE (XEXP (XEXP (x, 1), 0)) == AND)) > + return true; > + return false; > +} > + > diff --git a/gcc/ccmp.h b/gcc/ccmp.h > new file mode 100644 > index 0000000..7e139aa > --- /dev/null > +++ b/gcc/ccmp.h > @@ -0,0 +1,25 @@ > +/* Conditional comapre related functions. > + Copyright (C) 2014-2014 Free Software Foundation, Inc. > + > +This file is part of GCC. > + > +GCC is free software; you can redistribute it and/or modify it under > +the terms of the GNU General Public License as published by the Free > +Software Foundation; either version 3, or (at your option) any later > +version.: > + > +GCC is distributed in the hope that it will be useful, but WITHOUT ANY > +WARRANTY; without even the implied warranty of MERCHANTABILITY or > +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > +for more details. > + > +You should have received a copy of the GNU General Public License > +along with GCC; see the file COPYING3. If not see > +<http://www.gnu.org/licenses/>. */ > + > +#ifndef GCC_CCMP_H > +#define GCC_CCMP_H > + > +extern bool ccmp_insn_p (rtx); > + > +#endif /* GCC_CCMP_H */ > diff --git a/gcc/recog.c b/gcc/recog.c > index 8d10a4f..b53a28c 100644 > --- a/gcc/recog.c > +++ b/gcc/recog.c > @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see > #include "tree-pass.h" > #include "df.h" > #include "insn-codes.h" > +#include "ccmp.h" > > #ifndef STACK_PUSH_CODE > #ifdef STACK_GROWS_DOWNWARD > @@ -577,7 +578,8 @@ simplify_while_replacing (rtx *loc, rtx to, rtx object, > enum rtx_code code = GET_CODE (x); > rtx new_rtx = NULL_RTX; > > - if (SWAPPABLE_OPERANDS_P (x) > + /* Do not swap compares in conditional compare instruction. */ > + if (SWAPPABLE_OPERANDS_P (x) && !ccmp_insn_p (object) > && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1))) > { > validate_unshare_change (object, loc, >
On 06/25/14 08:44, Richard Earnshaw wrote: > On 23/06/14 07:58, Zhenqiang Chen wrote: >> Hi, >> >> Swapping operands in a ccmp will lead to illegal instructions. So the >> patch disables it in simplify_while_replacing. >> >> The patch is separated from >> https://gcc.gnu.org/ml/gcc-patches/2014-02/msg01407.html. >> >> To make it clean. The patch adds two files: ccmp.{c,h} to hold all new >> ccmp related functions. >> >> OK for trunk? >> >> Thanks! >> -Zhenqiang >> >> ChangeLog: >> 2014-06-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> * Makefile.in: Add ccmp.o >> * ccmp.c: New file. > > Do we really need a new file for one 10-line function? Seems like > overkill. I think it would be better to just drop this function into > recog.c. Right. And if we did want a new file, clearly the #includes need to be trimmed :-) Jeff
On 26 June 2014 05:03, Jeff Law <law@redhat.com> wrote: > On 06/25/14 08:44, Richard Earnshaw wrote: >> >> On 23/06/14 07:58, Zhenqiang Chen wrote: >>> >>> Hi, >>> >>> Swapping operands in a ccmp will lead to illegal instructions. So the >>> patch disables it in simplify_while_replacing. >>> >>> The patch is separated from >>> https://gcc.gnu.org/ml/gcc-patches/2014-02/msg01407.html. >>> >>> To make it clean. The patch adds two files: ccmp.{c,h} to hold all new >>> ccmp related functions. >>> >>> OK for trunk? >>> >>> Thanks! >>> -Zhenqiang >>> >>> ChangeLog: >>> 2014-06-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >>> >>> * Makefile.in: Add ccmp.o >>> * ccmp.c: New file. >> >> >> Do we really need a new file for one 10-line function? Seems like >> overkill. I think it would be better to just drop this function into >> recog.c. > > Right. And if we did want a new file, clearly the #includes need to be > trimmed :-) Yes. It is not necessary for this patch itself. The file is in need for "[PATCH, 4/10] expand ccmp". Overall it will have more than 300 lines of codes. And #includes are trimmed. Previously all codes were in expr.c. It always conflicted when rebasing. So I move them in separate files. Thanks! -Zhenqiang
On 25 June 2014 22:44, Richard Earnshaw <rearnsha@arm.com> wrote: > On 23/06/14 07:58, Zhenqiang Chen wrote: >> Hi, >> >> Swapping operands in a ccmp will lead to illegal instructions. So the >> patch disables it in simplify_while_replacing. >> >> The patch is separated from >> https://gcc.gnu.org/ml/gcc-patches/2014-02/msg01407.html. >> >> To make it clean. The patch adds two files: ccmp.{c,h} to hold all new >> ccmp related functions. >> >> OK for trunk? >> >> Thanks! >> -Zhenqiang >> >> ChangeLog: >> 2014-06-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> * Makefile.in: Add ccmp.o >> * ccmp.c: New file. > > Do we really need a new file for one 10-line function? Seems like > overkill. I think it would be better to just drop this function into > recog.c. > > Also, can you explain more clearly what the problem is with swapping the > operands? If this can't be done, then SWAPPABLE_OPERANDS is arguably > doing the wrong thing; and that might mean that rtx class you've applied > to your new code is incorrect. Thanks for the comments. In previous tests, I got several new fails if the operands were swapped. I will try to reproduce it and back to you. Thanks! -Zhenqiang > R. > >> * ccmp.h: New file. >> * recog.c (simplify_while_replacing): Check ccmp_insn_p. >> >> diff --git a/gcc/Makefile.in b/gcc/Makefile.in >> index 5587b75..8757a30 100644 >> --- a/gcc/Makefile.in >> +++ b/gcc/Makefile.in >> @@ -1169,6 +1169,7 @@ OBJS = \ >> builtins.o \ >> caller-save.o \ >> calls.o \ >> + ccmp.o \ >> cfg.o \ >> cfganal.o \ >> cfgbuild.o \ >> diff --git a/gcc/ccmp.c b/gcc/ccmp.c >> new file mode 100644 >> index 0000000..665c2a5 >> --- /dev/null >> +++ b/gcc/ccmp.c >> @@ -0,0 +1,62 @@ >> +/* Conditional compare related functions >> + Copyright (C) 2014-2014 Free Software Foundation, Inc. >> + >> +This file is part of GCC. >> + >> +GCC is free software; you can redistribute it and/or modify it under >> +the terms of the GNU General Public License as published by the Free >> +Software Foundation; either version 3, or (at your option) any later >> +version. >> + >> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY >> +WARRANTY; without even the implied warranty of MERCHANTABILITY or >> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> +for more details. >> + >> +You should have received a copy of the GNU General Public License >> +along with GCC; see the file COPYING3. If not see >> +<http://www.gnu.org/licenses/>. */ >> + >> +#include "config.h" >> +#include "system.h" >> +#include "coretypes.h" >> +#include "tm.h" >> +#include "rtl.h" >> +#include "tree.h" >> +#include "stringpool.h" >> +#include "regs.h" >> +#include "expr.h" >> +#include "optabs.h" >> +#include "tree-iterator.h" >> +#include "basic-block.h" >> +#include "tree-ssa-alias.h" >> +#include "internal-fn.h" >> +#include "gimple-expr.h" >> +#include "is-a.h" >> +#include "gimple.h" >> +#include "gimple-ssa.h" >> +#include "tree-ssanames.h" >> +#include "target.h" >> +#include "common/common-target.h" >> +#include "df.h" >> +#include "tree-ssa-live.h" >> +#include "tree-outof-ssa.h" >> +#include "cfgexpand.h" >> +#include "tree-phinodes.h" >> +#include "ssa-iterators.h" >> +#include "expmed.h" >> +#include "ccmp.h" >> + >> +bool >> +ccmp_insn_p (rtx object) >> +{ >> + rtx x = PATTERN (object); >> + if (targetm.gen_ccmp_first >> + && GET_CODE (x) == SET >> + && GET_CODE (XEXP (x, 1)) == COMPARE >> + && (GET_CODE (XEXP (XEXP (x, 1), 0)) == IOR >> + || GET_CODE (XEXP (XEXP (x, 1), 0)) == AND)) >> + return true; >> + return false; >> +} >> + >> diff --git a/gcc/ccmp.h b/gcc/ccmp.h >> new file mode 100644 >> index 0000000..7e139aa >> --- /dev/null >> +++ b/gcc/ccmp.h >> @@ -0,0 +1,25 @@ >> +/* Conditional comapre related functions. >> + Copyright (C) 2014-2014 Free Software Foundation, Inc. >> + >> +This file is part of GCC. >> + >> +GCC is free software; you can redistribute it and/or modify it under >> +the terms of the GNU General Public License as published by the Free >> +Software Foundation; either version 3, or (at your option) any later >> +version.: >> + >> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY >> +WARRANTY; without even the implied warranty of MERCHANTABILITY or >> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> +for more details. >> + >> +You should have received a copy of the GNU General Public License >> +along with GCC; see the file COPYING3. If not see >> +<http://www.gnu.org/licenses/>. */ >> + >> +#ifndef GCC_CCMP_H >> +#define GCC_CCMP_H >> + >> +extern bool ccmp_insn_p (rtx); >> + >> +#endif /* GCC_CCMP_H */ >> diff --git a/gcc/recog.c b/gcc/recog.c >> index 8d10a4f..b53a28c 100644 >> --- a/gcc/recog.c >> +++ b/gcc/recog.c >> @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see >> #include "tree-pass.h" >> #include "df.h" >> #include "insn-codes.h" >> +#include "ccmp.h" >> >> #ifndef STACK_PUSH_CODE >> #ifdef STACK_GROWS_DOWNWARD >> @@ -577,7 +578,8 @@ simplify_while_replacing (rtx *loc, rtx to, rtx object, >> enum rtx_code code = GET_CODE (x); >> rtx new_rtx = NULL_RTX; >> >> - if (SWAPPABLE_OPERANDS_P (x) >> + /* Do not swap compares in conditional compare instruction. */ >> + if (SWAPPABLE_OPERANDS_P (x) && !ccmp_insn_p (object) >> && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1))) >> { >> validate_unshare_change (object, loc, >> > >
diff --git a/gcc/Makefile.in b/gcc/Makefile.in index 5587b75..8757a30 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -1169,6 +1169,7 @@ OBJS = \ builtins.o \ caller-save.o \ calls.o \ + ccmp.o \ cfg.o \ cfganal.o \ cfgbuild.o \ diff --git a/gcc/ccmp.c b/gcc/ccmp.c new file mode 100644 index 0000000..665c2a5 --- /dev/null +++ b/gcc/ccmp.c @@ -0,0 +1,62 @@ +/* Conditional compare related functions + Copyright (C) 2014-2014 Free Software Foundation, Inc. + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify it under +the terms of the GNU General Public License as published by the Free +Software Foundation; either version 3, or (at your option) any later +version. + +GCC is distributed in the hope that it will be useful, but WITHOUT ANY +WARRANTY; without even the implied warranty of MERCHANTABILITY or +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +<http://www.gnu.org/licenses/>. */ + +#include "config.h" +#include "system.h" +#include "coretypes.h" +#include "tm.h" +#include "rtl.h" +#include "tree.h" +#include "stringpool.h" +#include "regs.h" +#include "expr.h" +#include "optabs.h" +#include "tree-iterator.h" +#include "basic-block.h" +#include "tree-ssa-alias.h" +#include "internal-fn.h" +#include "gimple-expr.h" +#include "is-a.h" +#include "gimple.h" +#include "gimple-ssa.h" +#include "tree-ssanames.h" +#include "target.h" +#include "common/common-target.h" +#include "df.h" +#include "tree-ssa-live.h" +#include "tree-outof-ssa.h" +#include "cfgexpand.h" +#include "tree-phinodes.h" +#include "ssa-iterators.h" +#include "expmed.h" +#include "ccmp.h" + +bool +ccmp_insn_p (rtx object) +{ + rtx x = PATTERN (object); + if (targetm.gen_ccmp_first + && GET_CODE (x) == SET + && GET_CODE (XEXP (x, 1)) == COMPARE + && (GET_CODE (XEXP (XEXP (x, 1), 0)) == IOR + || GET_CODE (XEXP (XEXP (x, 1), 0)) == AND)) + return true; + return false; +} + diff --git a/gcc/ccmp.h b/gcc/ccmp.h new file mode 100644 index 0000000..7e139aa --- /dev/null +++ b/gcc/ccmp.h @@ -0,0 +1,25 @@ +/* Conditional comapre related functions. + Copyright (C) 2014-2014 Free Software Foundation, Inc. + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify it under +the terms of the GNU General Public License as published by the Free +Software Foundation; either version 3, or (at your option) any later +version.: + +GCC is distributed in the hope that it will be useful, but WITHOUT ANY +WARRANTY; without even the implied warranty of MERCHANTABILITY or +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +<http://www.gnu.org/licenses/>. */ + +#ifndef GCC_CCMP_H +#define GCC_CCMP_H + +extern bool ccmp_insn_p (rtx); + +#endif /* GCC_CCMP_H */ diff --git a/gcc/recog.c b/gcc/recog.c index 8d10a4f..b53a28c 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-pass.h" #include "df.h" #include "insn-codes.h" +#include "ccmp.h" #ifndef STACK_PUSH_CODE #ifdef STACK_GROWS_DOWNWARD @@ -577,7 +578,8 @@ simplify_while_replacing (rtx *loc, rtx to, rtx object, enum rtx_code code = GET_CODE (x); rtx new_rtx = NULL_RTX; - if (SWAPPABLE_OPERANDS_P (x) + /* Do not swap compares in conditional compare instruction. */ + if (SWAPPABLE_OPERANDS_P (x) && !ccmp_insn_p (object) && swap_commutative_operands_p (XEXP (x, 0), XEXP (x, 1))) { validate_unshare_change (object, loc,
Hi, Swapping operands in a ccmp will lead to illegal instructions. So the patch disables it in simplify_while_replacing. The patch is separated from https://gcc.gnu.org/ml/gcc-patches/2014-02/msg01407.html. To make it clean. The patch adds two files: ccmp.{c,h} to hold all new ccmp related functions. OK for trunk? Thanks! -Zhenqiang ChangeLog: 2014-06-23 Zhenqiang Chen <zhenqiang.chen@linaro.org> * Makefile.in: Add ccmp.o * ccmp.c: New file. * ccmp.h: New file. * recog.c (simplify_while_replacing): Check ccmp_insn_p.