Re: PING [PATCH] RX movsicc degrade fix
On Mon, Feb 12, 2018 at 01:36:54PM +, Sebastian Perta wrote: > Hi Jakub, > > >>Still missing . at the end of the above line. > > The sentence continues on the next line (so the "." is there): > > +* config/rx/constraints.md (CALL_OP_SYMBOL_REF): Added new constraint > +to allow or block "symbol_ref" depending on the value of TARGET_JSR. > > I think this is OK, please confirm. You're right, confused by your mailer wrapping lines in between "new" and "constraint" around. Sorry. Jakub
Re: PING [PATCH] RX movsicc degrade fix
On Mon, Feb 12, 2018 at 01:27:24PM -, Sebastian Perta wrote: > --- ChangeLog (revision 257583) > +++ ChangeLog (working copy) > @@ -129,8 +129,7 @@ > > 2018-02-09 Sebastian Perta> > - * config/rx.md: updated "movsicc" expand to be matched by GCC > - * testsuite/gcc.target/rx/movsicc.c: new test case > + * config/rx/rx.md (movsicc): Update expander to be matched by GCC. > > 2018-02-09 Peter Bergner > > @@ -143,10 +142,10 @@ > > 2018-02-09 Sebastian Perta > > - * config/rx/constraints.md: added new constraint CALL_OP_SYMBOL_REF > - to allow or block "symbol_ref" depending on value of TARGET_JSR > - * config/rx/rx.md: use CALL_OP_SYMBOL_REF in call_internal and > - call_value_internal insns > + * config/rx/constraints.md (CALL_OP_SYMBOL_REF): Added new constraint Still missing . at the end of the above line. Otherwise LGTM. Jakub
RE: PING [PATCH] RX movsicc degrade fix
Hi Jakub, >>Still missing . at the end of the above line. The sentence continues on the next line (so the "." is there): +* config/rx/constraints.md (CALL_OP_SYMBOL_REF): Added new constraint +to allow or block "symbol_ref" depending on the value of TARGET_JSR. I think this is OK, please confirm. Best Regards, Sebastian > -Original Message- > From: Jakub Jelinek [mailto:ja...@redhat.com] > Sent: 12 February 2018 13:30 > To: Sebastian Perta <sebastian.pe...@renesas.com> > Cc: Nick Clifton <ni...@redhat.com>; gcc-patches patc...@gcc.gnu.org> > Subject: Re: PING [PATCH] RX movsicc degrade fix > > On Mon, Feb 12, 2018 at 01:27:24PM -, Sebastian Perta wrote: > > --- ChangeLog(revision 257583) > > +++ ChangeLog(working copy) > > @@ -129,8 +129,7 @@ > > > > 2018-02-09 Sebastian Perta <sebastian.pe...@renesas.com> > > > > -* config/rx.md: updated "movsicc" expand to be matched by GCC > > -* testsuite/gcc.target/rx/movsicc.c: new test case > > +* config/rx/rx.md (movsicc): Update expander to be matched by > GCC. > > > > 2018-02-09 Peter Bergner <berg...@vnet.ibm.com> > > > > @@ -143,10 +142,10 @@ > > > > 2018-02-09 Sebastian Perta <sebastian.pe...@renesas.com> > > > > -* config/rx/constraints.md: added new constraint > CALL_OP_SYMBOL_REF > > -to allow or block "symbol_ref" depending on value of TARGET_JSR > > -* config/rx/rx.md: use CALL_OP_SYMBOL_REF in call_internal and > > -call_value_internal insns > > +* config/rx/constraints.md (CALL_OP_SYMBOL_REF): Added new > constraint > > Still missing . at the end of the above line. > > Otherwise LGTM. > > Jakub Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
RE: PING [PATCH] RX movsicc degrade fix
a <sebastian.pe...@renesas.com> + + * gcc.target/rl78/test_addsi3_internal.c: New test. + 2018-01-26 Segher Boessenkool <seg...@kernel.crashing.org> * gcc.target/powerpc/safe-indirect-jump-1.c: Build on all targets. > -----Original Message- > From: Jakub Jelinek [mailto:ja...@redhat.com] > Sent: 12 February 2018 12:45 > To: Sebastian Perta <sebastian.pe...@renesas.com> > Cc: Nick Clifton <ni...@redhat.com>; gcc-patches patc...@gcc.gnu.org> > Subject: Re: PING [PATCH] RX movsicc degrade fix > > On Mon, Feb 12, 2018 at 11:06:35AM -, Sebastian Perta wrote: > > Hi Jakub, > > > > Thank you for pointing this out, I'm sorry! > > Can I create a patch to correct the changelog entries? > > > > Best Regards, > > Sebastian > > > > >>1) there should be a space between * and the filename > > The spaces are there (see the changelog), the renesas mail server removes > > them sometimes > > They weren't there in what I've fixed. > See > https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/ChangeLog?r1=257536=25753 > 5=257536 > > Jakub
Re: PING [PATCH] RX movsicc degrade fix
On Mon, Feb 12, 2018 at 11:06:35AM -, Sebastian Perta wrote: > Hi Jakub, > > Thank you for pointing this out, I'm sorry! > Can I create a patch to correct the changelog entries? > > Best Regards, > Sebastian > > >>1) there should be a space between * and the filename > The spaces are there (see the changelog), the renesas mail server removes > them sometimes They weren't there in what I've fixed. See https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/ChangeLog?r1=257536=257535=257536 Jakub
Re: PING [PATCH] RX movsicc degrade fix
On Mon, 2018-02-12 at 11:06 +, Sebastian Perta wrote: > > > 1) there should be a space between * and the filename > The spaces are there (see the changelog), the renesas mail server > removes them sometimes You might want to send around your patches as email attachments. That avoids formatting issues. Cheers, Oleg
Re: PING [PATCH] RX movsicc degrade fix
On Mon, Feb 12, 2018 at 11:06:35AM -, Sebastian Perta wrote: > Thank you for pointing this out, I'm sorry! > Can I create a patch to correct the changelog entries? Yes, and no need to add a ChangeLog entry for ChangeLog changes ;) Jakub
RE: PING [PATCH] RX movsicc degrade fix
Hi Jakub, Thank you for pointing this out, I'm sorry! Can I create a patch to correct the changelog entries? Best Regards, Sebastian >>1) there should be a space between * and the filename The spaces are there (see the changelog), the renesas mail server removes them sometimes > -Original Message- > From: Jakub Jelinek [mailto:ja...@redhat.com] > Sent: 09 February 2018 18:24 > To: Sebastian Perta <sebastian.pe...@renesas.com>; Nick Clifton > <ni...@redhat.com> > Cc: gcc-patches <gcc-patches@gcc.gnu.org> > Subject: Re: PING [PATCH] RX movsicc degrade fix > > On Wed, Feb 07, 2018 at 02:10:21PM +, Nick Clifton wrote: > > Hi Sebastian, > > > > Sorry for missing this one. If it helps in the future, feel free to ping me > directly. > > > > > +2018-01-09 Sebastian Perta <sebastian.pe...@renesas.com> > > > + > > > + *config/rx.md: updated "movsicc" expand to be matched by GCC > > > + *testsuite/gcc.target/rx/movsicc.c: new test case > > > > Approved - please apply. > > Note the ChangeLog is incorrect: > 1) there should be a space between * and the filename > 2) testsuite/ has its own ChangeLog, so changes for testsuite/ should >go there and filenames be relative to the testsuite/ directory > 3) there is no config/rx.md file, you've changed config/rx/rx.md instead > 4) the format is * filename (what): Description. , so it should be > * config/rx/rx.md (movsicc): Update expander to be matched by > GCC. > 5) note capital letter after : and full stop at the end. > * gcc.target/rx/movsicc.c: New test. >goes into testsuite/ChangeLog > > Many other of your ChangeLog entries suffer from similar issues. > > Jakub
Re: PING [PATCH] RX movsicc degrade fix
Hi Sebastian, Sorry for missing this one. If it helps in the future, feel free to ping me directly. > +2018-01-09 Sebastian Perta> + > + *config/rx.md: updated "movsicc" expand to be matched by GCC > + *testsuite/gcc.target/rx/movsicc.c: new test case Approved - please apply. Cheers Nick
PING [PATCH] RX movsicc degrade fix
PING > -Original Message- > From: Sebastian Perta [mailto:sebastian.pe...@renesas.com] > Sent: 09 January 2018 14:46 > To: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org> > Subject: [PATCH] RX movsicc degrade fix > > Hello, > > In recent versions of GCC the define_expand "movsicc" has stopped being > used by GCC (approx. 4.7.x/4.8.x onwards) > The reason for this is that the first operand of if_then_else has SI mode and > it shouldn't have. If we take a look at movsicc for all other targets we see this > is true. > The fix in rx.md is basically a copy paste from v850.md > > The patch also adds a testcase in gcc.target/rx to make sure this degrade > does not occur again. > > > Regression test is OK with one observation (see below), tested with the > following command: > make -k check-gcc RUNTESTFLAGS=--target_board=rx-sim > > > I have the following fail (which was not present before): > FAIL: gcc.dg/loop-8.c scan-rtl-dump-times loop2_invariant "Decided" 1 > (found 0 times) > > This is because the patch is effective in this test case and the dump is not the > same, I checked the asm code manually and it is OK. > Is it possible to disable parts of a test case, not the whole test case (* { dg- > final { scan-rtl-dump-times "Decided" 1 "loop2_invariant" } } */ from loop-8.c > in this example) for a particular target? > > The total numbers of failures remains the same because the following FAIL is > not present anymore with this patch: > FAIL: gcc.dg/ifcvt-4.c scan-rtl-dump ce1 "2 true changes made" > > > Please let me know if this is OK. Thank you! > > Best Regards, > Sebastian > > > Index: ChangeLog > == > = > --- ChangeLog (revision 256382) > +++ ChangeLog (working copy) > @@ -1,3 +1,8 @@ > +2018-01-09 Sebastian Perta <sebastian.pe...@renesas.com> > + > + *config/rx.md: updated "movsicc" expand to be matched by GCC > + *testsuite/gcc.target/rx/movsicc.c: new test case > + > 2018-01-09 Richard Biener <rguent...@suse.de> > > PR tree-optimization/83668 > Index: config/rx/rx.md > == > = > --- config/rx/rx.md (revision 256382) > +++ config/rx/rx.md (working copy) > @@ -733,12 +733,17 @@ > (define_expand "movsicc" >[(parallel > [(set (match_operand:SI 0 "register_operand") > - (if_then_else:SI (match_operand:SI 1 "comparison_operator") > + (if_then_else:SI (match_operand 1 "comparison_operator") > (match_operand:SI 2 "nonmemory_operand") > (match_operand:SI 3 "nonmemory_operand"))) > (clobber (reg:CC CC_REG))])] >"" > { > + /* Make sure that we have an integer comparison... */ > + if (GET_MODE (XEXP (operands[1], 0)) != CCmode > + && GET_MODE (XEXP (operands[1], 0)) != SImode) > +FAIL; > + >/* One operand must be a constant or a register, the other must be a > register. */ >if ( ! CONSTANT_P (operands[2]) >&& ! CONSTANT_P (operands[3]) > Index: testsuite/gcc.target/rx/movsicc.c > == > = > --- testsuite/gcc.target/rx/movsicc.c (nonexistent) > +++ testsuite/gcc.target/rx/movsicc.c (working copy) > @@ -0,0 +1,94 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os" } */ > + > +typedef unsigned char u8; > +typedef unsigned short u16; > +signed int Xa, Xb; > + > +signed int stzreg_beq(int i, int a, int b) > +{ > + signed int x; > + x = a; > + if (i) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "bne 1f" } } */ > + > +signed int stzreg_bge(int i, int a, int b, int c) > +{ > + signed int x; > + x = a; > + if (i<c) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "blt 1f" } } */ > + > +signed int stzreg_bgt(int i, int a, int b) > +{ > + signed int x; > + x = a; > + if (i<10) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "ble 1f" } } */ > + > +signed int stzreg_ble(int i, int a, int b) > +{ > + signed int x; > + x = a; > + if (i>0) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "bgt 1f" } } */ > + > +signed int stzreg_blt(int i, int a, int b) > +{ > + signed int x; > + x = a; > + if (i<0) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "blt 1f" } } */ > + > +signed int stzreg_bne(int i, int a, int b) > +{ > + signed int x; > + x = a; > + if (!i) > +x = b; > + return x; > +} > + > +/* { dg-final { scan-assembler "beq 1f" } } */ > + > +signed int stzimm_le( int i, int a ) > +{ > + signed int x; > + x = a; > + if (i>0) > +x = 5; > + return x; > +} > + > +/* { dg-final { scan-assembler "ble 1f" } } */ > + > +signed int stzimm_le_r( int i, int a ) > +{ > + signed int x; > + x = a; > + if (i<0) > +x = 5; > + return x; > +} > + > +/* { dg-final { scan-assembler "bge 1f" } } */
[PATCH] RX movsicc degrade fix
Hello, In recent versions of GCC the define_expand "movsicc" has stopped being used by GCC (approx. 4.7.x/4.8.x onwards) The reason for this is that the first operand of if_then_else has SI mode and it shouldn't have. If we take a look at movsicc for all other targets we see this is true. The fix in rx.md is basically a copy paste from v850.md The patch also adds a testcase in gcc.target/rx to make sure this degrade does not occur again. Regression test is OK with one observation (see below), tested with the following command: make -k check-gcc RUNTESTFLAGS=--target_board=rx-sim I have the following fail (which was not present before): FAIL: gcc.dg/loop-8.c scan-rtl-dump-times loop2_invariant "Decided" 1 (found 0 times) This is because the patch is effective in this test case and the dump is not the same, I checked the asm code manually and it is OK. Is it possible to disable parts of a test case, not the whole test case (* { dg-final { scan-rtl-dump-times "Decided" 1 "loop2_invariant" } } */ from loop-8.c in this example) for a particular target? The total numbers of failures remains the same because the following FAIL is not present anymore with this patch: FAIL: gcc.dg/ifcvt-4.c scan-rtl-dump ce1 "2 true changes made" Please let me know if this is OK. Thank you! Best Regards, Sebastian Index: ChangeLog === --- ChangeLog (revision 256382) +++ ChangeLog (working copy) @@ -1,3 +1,8 @@ +2018-01-09 Sebastian Perta+ + *config/rx.md: updated "movsicc" expand to be matched by GCC + *testsuite/gcc.target/rx/movsicc.c: new test case + 2018-01-09 Richard Biener PR tree-optimization/83668 Index: config/rx/rx.md === --- config/rx/rx.md (revision 256382) +++ config/rx/rx.md (working copy) @@ -733,12 +733,17 @@ (define_expand "movsicc" [(parallel [(set (match_operand:SI 0 "register_operand") - (if_then_else:SI (match_operand:SI 1 "comparison_operator") + (if_then_else:SI (match_operand 1 "comparison_operator") (match_operand:SI 2 "nonmemory_operand") (match_operand:SI 3 "nonmemory_operand"))) (clobber (reg:CC CC_REG))])] "" { + /* Make sure that we have an integer comparison... */ + if (GET_MODE (XEXP (operands[1], 0)) != CCmode + && GET_MODE (XEXP (operands[1], 0)) != SImode) +FAIL; + /* One operand must be a constant or a register, the other must be a register. */ if ( ! CONSTANT_P (operands[2]) && ! CONSTANT_P (operands[3]) Index: testsuite/gcc.target/rx/movsicc.c === --- testsuite/gcc.target/rx/movsicc.c (nonexistent) +++ testsuite/gcc.target/rx/movsicc.c (working copy) @@ -0,0 +1,94 @@ +/* { dg-do compile } */ +/* { dg-options "-Os" } */ + +typedef unsigned char u8; +typedef unsigned short u16; +signed int Xa, Xb; + +signed int stzreg_beq(int i, int a, int b) +{ + signed int x; + x = a; + if (i) +x = b; + return x; +} + +/* { dg-final { scan-assembler "bne 1f" } } */ + +signed int stzreg_bge(int i, int a, int b, int c) +{ + signed int x; + x = a; + if (i 0) +x = b; + return x; +} + +/* { dg-final { scan-assembler "bgt 1f" } } */ + +signed int stzreg_blt(int i, int a, int b) +{ + signed int x; + x = a; + if (i<0) +x = b; + return x; +} + +/* { dg-final { scan-assembler "blt 1f" } } */ + +signed int stzreg_bne(int i, int a, int b) +{ + signed int x; + x = a; + if (!i) +x = b; + return x; +} + +/* { dg-final { scan-assembler "beq 1f" } } */ + +signed int stzimm_le( int i, int a ) +{ + signed int x; + x = a; + if (i>0) +x = 5; + return x; +} + +/* { dg-final { scan-assembler "ble 1f" } } */ + +signed int stzimm_le_r( int i, int a ) +{ + signed int x; + x = a; + if (i<0) +x = 5; + return x; +} + +/* { dg-final { scan-assembler "bge 1f" } } */