| Message ID | 20191022081319.16165-1-claziss@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ARC] Fix movsi_ne pattern. | expand |
On 10/22/19 2:13 AM, Claudiu Zissulescu wrote: > From: Shahab Vahedi <shahab@synopsys.com> > > Hi Andrew, > > The movsi_ne variants are in a wrong order, leading to wrong > computation of the internal attribute "cond". Hence, to errors when > outputting annul-true or annul-false instructions. Testcase added. > > The patch needs to go for trunk and gcc9 branch. > > OK to apply? > Claudiu > > gcc/ > xxxx-xx-xx Claudiu Zissulescu <claziss@synopsys.com> > Shahab Vahedi <shahab@synopsys.com> > > * config/arc/arc.md (movsi_ne): Reorder instruction variants. > > testsuite/ > xxxx-xx-xx Shahab Vahedi <shahab@synopsys.com> > > * gcc.target/arc/delay-slot-limm.c: New test. So I'm going to have to trust you on this one. It looks like you did more than just reorder the alternatives. For example, the constraints for operand0 look significantly different to me. THey're slightly different for operand1 as well (LR rather than Lc). Anyway, OK for the trunk. jeff
Hi Jeff, > So I'm going to have to trust you on this one. It looks like you did > more than just reorder the alternatives. For example, the constraints > for operand0 look significantly different to me. THey're slightly > different for operand1 as well (LR rather than Lc). When we moved the ARC backend to LRA, we retired the register classes covered by `c` and `w` register constraints letters. Now, all of them are pointing to the same register class like `r`. So, whenever I have the opportunity, I also refactor the patterns not to use any longer the obsolete register constraints. Probably, this is the reason why the patch looks like it is more than reordering, but at the core it is just that :) Thank you for your review, Claudiu
On 10/30/19 3:58 AM, Claudiu Zissulescu wrote: > Hi Jeff, > >> So I'm going to have to trust you on this one. It looks like you did >> more than just reorder the alternatives. For example, the constraints >> for operand0 look significantly different to me. THey're slightly >> different for operand1 as well (LR rather than Lc). > > When we moved the ARC backend to LRA, we retired the register classes covered by `c` and `w` register constraints letters. > Now, all of them are pointing to the same register class like `r`. > So, whenever I have the opportunity, I also refactor the patterns not to use any longer the obsolete register constraints. Probably, this is the reason why the patch looks like it is more than reordering, but at the core it is just that :) Thanks for the explanation. I'll keep in mind for other ARC patches as well. jeff
diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 9cfde19582c..0bc8ce5378f 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -3782,20 +3782,20 @@ archs4x, archs4xd" ; cond_exec patterns (define_insn "*movsi_ne" [(cond_exec - (ne (match_operand:CC_Z 2 "cc_use_register" "Rcc, Rcc, Rcc,Rcc,Rcc") (const_int 0)) - (set (match_operand:SI 0 "dest_reg_operand" "=Rcq#q,Rcq#q,Rcq#q, w,w") - (match_operand:SI 1 "nonmemory_operand" "C_0, h, ?Cal, Lc,?Cal")))] + (ne (match_operand:CC_Z 2 "cc_use_register" "Rcc,Rcc,Rcc,Rcc,Rcc") (const_int 0)) + (set (match_operand:SI 0 "dest_reg_operand" "=q, q, r, q, r") + (match_operand:SI 1 "nonmemory_operand" "C_0, h, Lr,Cal,Cal")))] "" "@ - * current_insn_predicate = 0; return \"sub%?.ne %0,%0,%0%&\"; - * current_insn_predicate = 0; return \"mov%?.ne %0,%1\"; - * current_insn_predicate = 0; return \"mov%?.ne %0,%1\"; - mov.ne %0,%1 - mov.ne %0,%1" + * current_insn_predicate = 0; return \"sub%?.ne\\t%0,%0,%0\"; + * current_insn_predicate = 0; return \"mov%?.ne\\t%0,%1\"; + mov.ne\\t%0,%1 + * current_insn_predicate = 0; return \"mov%?.ne\\t%0,%1\"; + mov.ne\\t%0,%1" [(set_attr "type" "cmove") - (set_attr "iscompact" "true,true,true_limm,false,false") - (set_attr "length" "2,2,6,4,8") - (set_attr "cpu_facility" "*,av2,av2,*,*")]) + (set_attr "iscompact" "true,true,false,true_limm,false") + (set_attr "length" "2,2,4,6,8") + (set_attr "cpu_facility" "*,av2,*,av2,*")]) (define_insn "*movsi_cond_exec" [(cond_exec diff --git a/gcc/testsuite/gcc.target/arc/delay-slot-limm.c b/gcc/testsuite/gcc.target/arc/delay-slot-limm.c new file mode 100644 index 00000000000..e5de3c4badd --- /dev/null +++ b/gcc/testsuite/gcc.target/arc/delay-slot-limm.c @@ -0,0 +1,52 @@ +/* We have encountered an issue that a "mov_s.ne" instruction * + * with an immediate value was put in the delay slot of a * + * branch: * + * * + * bne.d @.L1 # 33 [c=20 l=4] *branch_insn * + * mov_s.ne r0,7 # 35 [c=0 l=6] *movsi_ne/2 * + * * + * This is not sanctioned and must not happen. The test below * + * is a reduced version of the source code leading to the * + * problem. */ + +/* { dg-do compile } */ +/* { dg-skip-if "" { ! { clmcpu } } } */ +/* { dg-options "-mcpu=archs -Og" } */ +typedef struct +{ + struct + { + int length; + } table; +} room; + +struct house +{ + room *r; +}; + +int glob; + +_Bool bar(); + +int func(struct house *h, int i, int whatever) +{ + for (;;) + { + _Bool a; + if (h && h->r[i].table.length == glob) + { + if (whatever) + { + a = bar(); + if (__builtin_expect(!a, 0)) + return 7; + } + break; + } + } + return 0; +} + +/* no 'mov_s.ne r,limm' in a delay slot */ +/* { dg-final { scan-assembler-not "bne.d\.*\n\\s\+mov_s.ne\\s+r\[0-9\]+,7" } } */
From: Shahab Vahedi <shahab@synopsys.com> Hi Andrew, The movsi_ne variants are in a wrong order, leading to wrong computation of the internal attribute "cond". Hence, to errors when outputting annul-true or annul-false instructions. Testcase added. The patch needs to go for trunk and gcc9 branch. OK to apply? Claudiu gcc/ xxxx-xx-xx Claudiu Zissulescu <claziss@synopsys.com> Shahab Vahedi <shahab@synopsys.com> * config/arc/arc.md (movsi_ne): Reorder instruction variants. testsuite/ xxxx-xx-xx Shahab Vahedi <shahab@synopsys.com> * gcc.target/arc/delay-slot-limm.c: New test. --- gcc/config/arc/arc.md | 22 ++++---- .../gcc.target/arc/delay-slot-limm.c | 52 +++++++++++++++++++ 2 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/gcc.target/arc/delay-slot-limm.c