* [patch] Performance patch for MIPS conditional move in expr.c @ 2012-11-14 19:02 Steve Ellcey 2012-11-14 19:15 ` Andrew Pinski 2012-11-15 1:51 ` Richard Henderson 0 siblings, 2 replies; 14+ messages in thread From: Steve Ellcey @ 2012-11-14 19:02 UTC (permalink / raw) To: gcc-patches; +Cc: rguenther Back in August of 2011, Richard Biener made a change affecting COND_EXPRs (r178408). As a result of that change the GCC MIPS compiler that used to promote HImode variables to SImode and then put out SImode variables and assignments for the conditional move (MIPS doesn't support HImode conditional moves) started putting out HImode variables and assignments and the resulting code is slower then it was. The code that used to look like this: (insn 23 22 24 3 (set (reg/v:SI 231 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 320) 2))) x.c:11 -1 (nil)) (insn 27 26 28 3 (set (reg/v:SI 233 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 323) 2))) x.c:12 -1 (nil)) IF () (insn 30 241 31 4 (set (reg:SI 324) (reg/v:SI 231 [ a2+-2 ])) -1 (nil)) ELSE (insn 34 242 35 5 (set (reg:SI 324) (reg/v:SI 233 [ a2+-2 ])) -1 (nil)) (insn 36 243 37 6 (set (reg/v:SI 234 [ a2+-2 ]) (reg:SI 324)) -1 (nil)) started outputting HI assignments instead: (insn 23 22 24 3 (set (reg/v:SI 231 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 320) 2))) x.c:11 -1 (nil)) (insn 27 26 28 3 (set (reg/v:SI 233 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 323) 2))) x.c:12 -1 (nil)) IF () (insn 30 241 31 4 (set (reg:HI 324) (subreg/s/u:HI (reg/v:SI 231 [ a2+-2 ]) 2)) -1 (nil)) ELSE (insn 34 242 35 5 (set (reg:HI 324) (subreg/s/u:HI (reg/v:SI 233 [ a2+-2 ]) 2)) -1 (nil)) (insn 36 243 37 6 (set (reg/v:SI 234 [ a2+-2 ]) (zero_extend:SI (reg:HI 324))) -1 (nil)) This resulted in an extra 'andi REG,REG,0xffff' instruction to implement the final zero_extend instruction and that slowed the code down. This patch restores the previous behaviour by modifying expand_cond_expr_using_cmove so that when we need to promote the mode in order to do a conditional move we use that promoted mode in the temp that we are creating for the conditional move. Tested on mips-mti-elf with no regressions. OK for checkin? Steve Ellcey sellcey@mips.com 2012-11-14 Steve Ellcey <sellcey@mips.com> * expr.c (expand_cond_expr_using_cmove): Use promoted mode for temp. diff --git a/gcc/expr.c b/gcc/expr.c index cbf3a40..b1b83d0 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -7840,15 +7840,17 @@ expand_cond_expr_using_cmove (tree treeop0 ATTRIBUTE_UNUSED, int unsignedp = TYPE_UNSIGNED (type); enum machine_mode mode = TYPE_MODE (type); - temp = assign_temp (type, 0, 1); - /* If we cannot do a conditional move on the mode, try doing it with the promoted mode. */ if (!can_conditionally_move_p (mode)) - mode = promote_mode (type, mode, &unsignedp); - - if (!can_conditionally_move_p (mode)) - return NULL_RTX; + { + mode = promote_mode (type, mode, &unsignedp); + if (!can_conditionally_move_p (mode)) + return NULL_RTX; + temp = assign_temp (type, 0, 0); /* Use promoted mode for temp. */ + } + else + temp = assign_temp (type, 0, 1); start_sequence (); expand_operands (treeop1, treeop2, ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 19:02 [patch] Performance patch for MIPS conditional move in expr.c Steve Ellcey @ 2012-11-14 19:15 ` Andrew Pinski 2012-11-14 19:27 ` Steve Ellcey 2012-11-15 1:51 ` Richard Henderson 1 sibling, 1 reply; 14+ messages in thread From: Andrew Pinski @ 2012-11-14 19:15 UTC (permalink / raw) To: Steve Ellcey; +Cc: gcc-patches, rguenther On Wed, Nov 14, 2012 at 11:02 AM, Steve Ellcey <sellcey@mips.com> wrote: > > Back in August of 2011, Richard Biener made a change affecting COND_EXPRs > (r178408). As a result of that change the GCC MIPS compiler that used > to promote HImode variables to SImode and then put out SImode variables > and assignments for the conditional move (MIPS doesn't support HImode > conditional moves) started putting out HImode variables and assignments and > the resulting code is slower then it was. > > The code that used to look like this: > > (insn 23 22 24 3 (set (reg/v:SI 231 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 320) 2))) x.c:11 -1 (nil)) > (insn 27 26 28 3 (set (reg/v:SI 233 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 323) 2))) x.c:12 -1 (nil)) > IF () > (insn 30 241 31 4 (set (reg:SI 324) (reg/v:SI 231 [ a2+-2 ])) -1 (nil)) > ELSE > (insn 34 242 35 5 (set (reg:SI 324) (reg/v:SI 233 [ a2+-2 ])) -1 (nil)) > (insn 36 243 37 6 (set (reg/v:SI 234 [ a2+-2 ]) (reg:SI 324)) -1 (nil)) > > > > started outputting HI assignments instead: > > > > (insn 23 22 24 3 (set (reg/v:SI 231 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 320) 2))) x.c:11 -1 (nil)) > (insn 27 26 28 3 (set (reg/v:SI 233 [ a2+-2 ]) (zero_extend:SI (subreg:HI (reg:SI 323) 2))) x.c:12 -1 (nil)) > IF () > (insn 30 241 31 4 (set (reg:HI 324) (subreg/s/u:HI (reg/v:SI 231 [ a2+-2 ]) 2)) -1 (nil)) > ELSE > (insn 34 242 35 5 (set (reg:HI 324) (subreg/s/u:HI (reg/v:SI 233 [ a2+-2 ]) 2)) -1 (nil)) > (insn 36 243 37 6 (set (reg/v:SI 234 [ a2+-2 ]) (zero_extend:SI (reg:HI 324))) -1 (nil)) > > This resulted in an extra 'andi REG,REG,0xffff' instruction to implement the > final zero_extend instruction and that slowed the code down. This patch > restores the previous behaviour by modifying expand_cond_expr_using_cmove > so that when we need to promote the mode in order to do a conditional move > we use that promoted mode in the temp that we are creating for the conditional > move. > > Tested on mips-mti-elf with no regressions. > > OK for checkin? Do you have a testcase? As I added expand_cond_expr_using_cmove, I think this is the correct fix. Thanks, Andrew Pinski > > Steve Ellcey > sellcey@mips.com > > > 2012-11-14 Steve Ellcey <sellcey@mips.com> > > * expr.c (expand_cond_expr_using_cmove): Use promoted mode for temp. > > > diff --git a/gcc/expr.c b/gcc/expr.c > index cbf3a40..b1b83d0 100644 > --- a/gcc/expr.c > +++ b/gcc/expr.c > @@ -7840,15 +7840,17 @@ expand_cond_expr_using_cmove (tree treeop0 ATTRIBUTE_UNUSED, > int unsignedp = TYPE_UNSIGNED (type); > enum machine_mode mode = TYPE_MODE (type); > > - temp = assign_temp (type, 0, 1); > - > /* If we cannot do a conditional move on the mode, try doing it > with the promoted mode. */ > if (!can_conditionally_move_p (mode)) > - mode = promote_mode (type, mode, &unsignedp); > - > - if (!can_conditionally_move_p (mode)) > - return NULL_RTX; > + { > + mode = promote_mode (type, mode, &unsignedp); > + if (!can_conditionally_move_p (mode)) > + return NULL_RTX; > + temp = assign_temp (type, 0, 0); /* Use promoted mode for temp. */ > + } > + else > + temp = assign_temp (type, 0, 1); > > start_sequence (); > expand_operands (treeop1, treeop2, ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 19:15 ` Andrew Pinski @ 2012-11-14 19:27 ` Steve Ellcey 2012-11-14 20:01 ` Andrew Pinski 0 siblings, 1 reply; 14+ messages in thread From: Steve Ellcey @ 2012-11-14 19:27 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, rguenther On Wed, 2012-11-14 at 11:15 -0800, Andrew Pinski wrote: > Do you have a testcase? As I added expand_cond_expr_using_cmove, I > think this is the correct fix. > > Thanks, > Andrew Pinski Here is a cutdown test case that I have been compiling with -O3, if you compare with and without my patch you will see fewer 'andi' instructions with 0xffff are generated after my patch is applied. Steve Ellcey sellcey@mips.com unsigned short foo(unsigned short a1, unsigned short a2) { unsigned short i, x; for (i = 0; i < 8; i++) { x = (a1 & 1) ^ (a2 & 1); a1 >>= 1; if (x == 1) a2 ^= 0x2006; a2 >>= 1; if (x == 1) a2 |= 0x8800; else a2 &= 0x77ff; } return a2; } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 19:27 ` Steve Ellcey @ 2012-11-14 20:01 ` Andrew Pinski 2012-11-14 21:46 ` Steve Ellcey 0 siblings, 1 reply; 14+ messages in thread From: Andrew Pinski @ 2012-11-14 20:01 UTC (permalink / raw) To: Steve Ellcey; +Cc: gcc-patches, rguenther On Wed, Nov 14, 2012 at 11:27 AM, Steve Ellcey <sellcey@mips.com> wrote: > On Wed, 2012-11-14 at 11:15 -0800, Andrew Pinski wrote: > >> Do you have a testcase? As I added expand_cond_expr_using_cmove, I >> think this is the correct fix. >> >> Thanks, >> Andrew Pinski > > Here is a cutdown test case that I have been compiling with -O3, if you > compare with and without my patch you will see fewer 'andi' instructions > with 0xffff are generated after my patch is applied. I know exactly where this code comes from; I have looked at the benchmark as one of the reason why I add expand_cond_expr_using_cmove in the first place. Anyways you should look into removing TARGET_PROMOTE_PROTOTYPES because I found that also fixes the problem mentioned here. Thanks, Andrew Pinski > > Steve Ellcey > sellcey@mips.com > > unsigned short foo(unsigned short a1, unsigned short a2) > { > unsigned short i, x; > for (i = 0; i < 8; i++) { > x = (a1 & 1) ^ (a2 & 1); > a1 >>= 1; > if (x == 1) a2 ^= 0x2006; > a2 >>= 1; > if (x == 1) a2 |= 0x8800; > else a2 &= 0x77ff; > } > return a2; > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 20:01 ` Andrew Pinski @ 2012-11-14 21:46 ` Steve Ellcey 2012-11-14 21:51 ` Andrew Pinski 0 siblings, 1 reply; 14+ messages in thread From: Steve Ellcey @ 2012-11-14 21:46 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, rguenther On Wed, 2012-11-14 at 12:00 -0800, Andrew Pinski wrote: > I know exactly where this code comes from; I have looked at the > benchmark as one of the reason why I add expand_cond_expr_using_cmove > in the first place. Anyways you should look into removing > TARGET_PROMOTE_PROTOTYPES because I found that also fixes the problem > mentioned here. > > Thanks, > Andrew Pinski Removing TARGET_PROMOTE_PROTOTYPES looks interesting but I don't know if it is possible for compatibility reasons. I am still looking at my example though, I see GCC doing: andi $5,$5,0x1 xori $5,$5,0x1 movz $2,$4,$5 When it should just do: andi $5,$5,0x1 movn $2,$4,$5 Steve Ellcey sellcey@mips.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 21:46 ` Steve Ellcey @ 2012-11-14 21:51 ` Andrew Pinski 2012-11-14 22:22 ` Andrew Pinski 0 siblings, 1 reply; 14+ messages in thread From: Andrew Pinski @ 2012-11-14 21:51 UTC (permalink / raw) To: Steve Ellcey; +Cc: gcc-patches, rguenther [-- Attachment #1: Type: text/plain, Size: 1157 bytes --] On Wed, Nov 14, 2012 at 1:45 PM, Steve Ellcey <sellcey@mips.com> wrote: > On Wed, 2012-11-14 at 12:00 -0800, Andrew Pinski wrote: > >> I know exactly where this code comes from; I have looked at the >> benchmark as one of the reason why I add expand_cond_expr_using_cmove >> in the first place. Anyways you should look into removing >> TARGET_PROMOTE_PROTOTYPES because I found that also fixes the problem >> mentioned here. >> >> Thanks, >> Andrew Pinski > > Removing TARGET_PROMOTE_PROTOTYPES looks interesting but I don't know if > it is possible for compatibility reasons. I am still looking at my > example though, I see GCC doing: > > andi $5,$5,0x1 > xori $5,$5,0x1 > movz $2,$4,$5 > > When it should just do: > > andi $5,$5,0x1 > movn $2,$4,$5 Yes I have a few patches for improving this case. I have not submitted them yet though. Attached is the assembly I get from a 4.7 toolchain with all of the changes I have internally applied; this is for n32 (though o32 produces the exact same code in this case). I will try to post some more in the next coming weeks. Thanks, Andrew Pinski > > > Steve Ellcey > sellcey@mips.com > > [-- Attachment #2: n32.s --] [-- Type: application/octet-stream, Size: 1655 bytes --] .file 1 "t5.c" .section .mdebug.abiN32 .previous .gnu_attribute 4, 3 .abicalls .option pic0 .text .align 2 .align 3 .globl foo .LFB0 = . .cfi_startproc .set nomips16 .ent foo .type foo, @function foo: .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 .mask 0x00000000,0 .fmask 0x00000000,0 .set noreorder .set nomacro xori $3,$5,0x2006 xor $6,$5,$4 srl $7,$3,1 srl $2,$5,1 ori $8,$7,0x8800 andi $5,$6,0x1 andi $9,$2,0x77ff srl $4,$4,1 movn $9,$8,$5 srl $10,$4,1 srl $11,$10,1 xori $12,$9,0x2006 xor $13,$9,$4 srl $14,$12,1 srl $15,$9,1 andi $24,$13,0x1 ori $25,$14,0x8800 andi $6,$15,0x77ff srl $5,$11,1 movn $6,$25,$24 srl $4,$5,1 srl $3,$4,1 xori $2,$6,0x2006 xor $7,$6,$10 srl $8,$2,1 srl $10,$6,1 andi $12,$7,0x1 ori $9,$8,0x8800 andi $13,$10,0x77ff srl $14,$3,1 movn $13,$9,$12 xori $15,$13,0x2006 xor $11,$13,$11 srl $24,$15,1 srl $25,$13,1 andi $6,$11,0x1 ori $7,$24,0x8800 andi $2,$25,0x77ff movn $2,$7,$6 xori $8,$2,0x2006 xor $5,$2,$5 srl $10,$8,1 srl $12,$2,1 andi $9,$5,0x1 ori $13,$10,0x8800 andi $15,$12,0x77ff movn $15,$13,$9 xori $11,$15,0x2006 xor $4,$15,$4 srl $24,$11,1 srl $25,$15,1 andi $6,$4,0x1 ori $7,$24,0x8800 andi $2,$25,0x77ff movn $2,$7,$6 xori $8,$2,0x2006 xor $3,$2,$3 srl $10,$2,1 srl $5,$8,1 andi $12,$3,0x1 ori $9,$5,0x8800 andi $13,$10,0x77ff movn $13,$9,$12 xori $15,$13,0x2006 xor $14,$13,$14 srl $11,$15,1 srl $24,$13,1 andi $25,$14,0x1 ori $4,$11,0x8800 andi $2,$24,0x77ff j $31 movn $2,$4,$25 .set macro .set reorder .end foo .cfi_endproc .LFE0: .size foo, .-foo .ident "GCC: (Cavium Development Version) 4.7.0" ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 21:51 ` Andrew Pinski @ 2012-11-14 22:22 ` Andrew Pinski 2012-11-15 20:59 ` Richard Sandiford 2013-03-07 15:12 ` Jakub Jelinek 0 siblings, 2 replies; 14+ messages in thread From: Andrew Pinski @ 2012-11-14 22:22 UTC (permalink / raw) To: Steve Ellcey; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1716 bytes --] On Wed, Nov 14, 2012 at 1:51 PM, Andrew Pinski <andrew.pinski@caviumnetworks.com> wrote: > On Wed, Nov 14, 2012 at 1:45 PM, Steve Ellcey <sellcey@mips.com> wrote: >> On Wed, 2012-11-14 at 12:00 -0800, Andrew Pinski wrote: >> >>> I know exactly where this code comes from; I have looked at the >>> benchmark as one of the reason why I add expand_cond_expr_using_cmove >>> in the first place. Anyways you should look into removing >>> TARGET_PROMOTE_PROTOTYPES because I found that also fixes the problem >>> mentioned here. >>> >>> Thanks, >>> Andrew Pinski >> >> Removing TARGET_PROMOTE_PROTOTYPES looks interesting but I don't know if >> it is possible for compatibility reasons. I am still looking at my >> example though, I see GCC doing: >> >> andi $5,$5,0x1 >> xori $5,$5,0x1 >> movz $2,$4,$5 >> >> When it should just do: >> >> andi $5,$5,0x1 >> movn $2,$4,$5 > > Yes I have a few patches for improving this case. I have not > submitted them yet though. > Attached is the assembly I get from a 4.7 toolchain with all of the > changes I have internally applied; this is for n32 (though o32 > produces the exact same code in this case). I will try to post some > more in the next coming weeks. Removing Richard B. from the CC list since this is now a MIPS specific change (the original patch is still needed though). Here is the patch which should improve the above case. I have not tested it on the trunk yet but it applies there. Basically the *mov<GPR:mode>_on_<GPR2:mode>_ne pattern is the one which is needed in this case. The other two changes are done for 64bit comparisons. Thanks, Andrew Pinski > > Thanks, > Andrew Pinski > > >> >> >> Steve Ellcey >> sellcey@mips.com >> >> [-- Attachment #2: improvemips_cond_expr.diff.txt --] [-- Type: text/plain, Size: 2510 bytes --] commit 8ca1e58de404bbe82b93bc240ef28c68c681243d Author: Andrew Pinski <apinski@cavium.com> Date: Thu Jul 26 18:09:34 2012 -0700 2012-07-26 Andrew Pinski <apinski@cavium.com> Bug #3261 * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): Remove mode check from comparisons. (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match when (ne A 0) can be just A. * testsuite/gcc.target/mips/movcc-4.c: New testcase. diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md index 0dff58e..a1e9568 100644 --- a/gcc/config/mips/mips.md +++ b/gcc/config/mips/mips.md @@ -6765,7 +6765,7 @@ (define_insn "*mov<GPR:mode>_on_<MOVECC:mode>" [(set (match_operand:GPR 0 "register_operand" "=d,d") (if_then_else:GPR - (match_operator:MOVECC 4 "equality_operator" + (match_operator 4 "equality_operator" [(match_operand:MOVECC 1 "register_operand" "<MOVECC:reg>,<MOVECC:reg>") (const_int 0)]) (match_operand:GPR 2 "reg_or_0_operand" "dJ,0") @@ -6777,10 +6777,23 @@ [(set_attr "type" "condmove") (set_attr "mode" "<GPR:MODE>")]) +(define_insn "*mov<GPR:mode>_on_<GPR2:mode>_ne" + [(set (match_operand:GPR 0 "register_operand" "=d,d") + (if_then_else:GPR + (match_operand:GPR2 1 "register_operand" "<GPR2:reg>,<GPR2:reg>") + (match_operand:GPR 2 "reg_or_0_operand" "dJ,0") + (match_operand:GPR 3 "reg_or_0_operand" "0,dJ")))] + "ISA_HAS_CONDMOVE" + "@ + movn\t%0,%z2,%1 + movz\t%0,%z3,%1" + [(set_attr "type" "condmove") + (set_attr "mode" "<GPR:MODE>")]) + (define_insn "*mov<SCALARF:mode>_on_<MOVECC:mode>" [(set (match_operand:SCALARF 0 "register_operand" "=f,f") (if_then_else:SCALARF - (match_operator:MOVECC 4 "equality_operator" + (match_operator 4 "equality_operator" [(match_operand:MOVECC 1 "register_operand" "<MOVECC:reg>,<MOVECC:reg>") (const_int 0)]) (match_operand:SCALARF 2 "register_operand" "f,0") diff --git a/gcc/testsuite/gcc.target/mips/movcc-4.c b/gcc/testsuite/gcc.target/mips/movcc-4.c new file mode 100644 index 0000000..d364a52 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/movcc-4.c @@ -0,0 +1,13 @@ +/* { dg-options "-O2 isa>=4" } */ +/* { dg-final { scan-assembler-times "movz\t|movn\t" 1 } } */ +/* { dg-final { scan-assembler-not "bbit0\t|bbit1\t" } } */ +/* { dg-final { scan-assembler-not "xori\t|xor\t" } } */ + +NOMIPS16 int f(int a, int b, int c) +{ + int d = a&0x1; + if (d==1) + return b; + return c; +} + ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 22:22 ` Andrew Pinski @ 2012-11-15 20:59 ` Richard Sandiford 2012-11-15 21:24 ` Andrew Pinski 2013-03-07 15:12 ` Jakub Jelinek 1 sibling, 1 reply; 14+ messages in thread From: Richard Sandiford @ 2012-11-15 20:59 UTC (permalink / raw) To: Andrew Pinski; +Cc: Steve Ellcey, gcc-patches Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > 2012-07-26 Andrew Pinski <apinski@cavium.com> > > Bug #3261 > * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): > Remove mode check from comparisons. > (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. > (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match > when (ne A 0) can be just A. > > * testsuite/gcc.target/mips/movcc-4.c: New testcase. OK, thanks (but remember to remove the internal bug reference :-)). I think this is early enough during stage 3 for the usual target flexibility to apply. Richard ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-15 20:59 ` Richard Sandiford @ 2012-11-15 21:24 ` Andrew Pinski 2012-11-15 21:39 ` Andrew Pinski 0 siblings, 1 reply; 14+ messages in thread From: Andrew Pinski @ 2012-11-15 21:24 UTC (permalink / raw) To: Andrew Pinski, Steve Ellcey, gcc-patches, rdsandiford On Thu, Nov 15, 2012 at 12:58 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: >> 2012-07-26 Andrew Pinski <apinski@cavium.com> >> >> Bug #3261 >> * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): >> Remove mode check from comparisons. >> (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. >> (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match >> when (ne A 0) can be just A. >> >> * testsuite/gcc.target/mips/movcc-4.c: New testcase. > > OK, thanks (but remember to remove the internal bug reference :-)). > I think this is early enough during stage 3 for the usual target > flexibility to apply. I was posting it for Steve's benefit really. I was in the process of updating the patch to the trunk and trying it out there before doing a formal submission :). As I found out the testcase needs to be changed to work with the new mips target test infrastructure. I will post a revised patch with the removal of the internal bug number once I finish fixing the testcase itself. Thanks, Andrew Pinski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-15 21:24 ` Andrew Pinski @ 2012-11-15 21:39 ` Andrew Pinski 2013-01-07 21:39 ` Steve Ellcey 0 siblings, 1 reply; 14+ messages in thread From: Andrew Pinski @ 2012-11-15 21:39 UTC (permalink / raw) To: Andrew Pinski, Steve Ellcey, gcc-patches, rdsandiford On Thu, Nov 15, 2012 at 1:24 PM, Andrew Pinski <andrew.pinski@caviumnetworks.com> wrote: > On Thu, Nov 15, 2012 at 12:58 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: >>> 2012-07-26 Andrew Pinski <apinski@cavium.com> >>> >>> Bug #3261 >>> * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): >>> Remove mode check from comparisons. >>> (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. >>> (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match >>> when (ne A 0) can be just A. >>> >>> * testsuite/gcc.target/mips/movcc-4.c: New testcase. >> >> OK, thanks (but remember to remove the internal bug reference :-)). >> I think this is early enough during stage 3 for the usual target >> flexibility to apply. > > I was posting it for Steve's benefit really. I was in the process of > updating the patch to the trunk and trying it out there before doing a > formal submission :). As I found out the testcase needs to be changed > to work with the new mips target test infrastructure. I will post a > revised patch with the removal of the internal bug number once I > finish fixing the testcase itself. After fixing up the testcase I find xori still in the resulting code: gcc$ ./cc1 t.c -O1 -o - -DNOMIPS16= -quiet -mabi=n32 -march=mips64 |grep xor xori $2,$4,0x1 But that is because combine produces: Trying 34 -> 35: Failed to match this instruction: (set (reg:SI 194 [ D.1393 ]) (if_then_else:SI (xor:SI (reg:SI 200 [ d ]) (const_int 1 [0x1])) (reg:SI 6 $6 [ c ]) (reg:SI 5 $5 [ b ]))) But does not switch around the if_then_else as reg 200 has a non zero bits of just 1. I will look into fix the rest of the problem later today. So the above patch is a way forward but not the full fix. Thanks, Andrew Pinski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-15 21:39 ` Andrew Pinski @ 2013-01-07 21:39 ` Steve Ellcey 0 siblings, 0 replies; 14+ messages in thread From: Steve Ellcey @ 2013-01-07 21:39 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches, rdsandiford On Thu, 2012-11-15 at 13:39 -0800, Andrew Pinski wrote: > > I was posting it for Steve's benefit really. I was in the process of > > updating the patch to the trunk and trying it out there before doing a > > formal submission :). As I found out the testcase needs to be changed > > to work with the new mips target test infrastructure. I will post a > > revised patch with the removal of the internal bug number once I > > finish fixing the testcase itself. > > After fixing up the testcase I find xori still in the resulting code: > gcc$ ./cc1 t.c -O1 -o - -DNOMIPS16= -quiet -mabi=n32 -march=mips64 |grep xor > xori $2,$4,0x1 > > But that is because combine produces: > Trying 34 -> 35: > Failed to match this instruction: > (set (reg:SI 194 [ D.1393 ]) > (if_then_else:SI (xor:SI (reg:SI 200 [ d ]) > (const_int 1 [0x1])) > (reg:SI 6 $6 [ c ]) > (reg:SI 5 $5 [ b ]))) > > But does not switch around the if_then_else as reg 200 has a non zero > bits of just 1. I will look into fix the rest of the problem later > today. So the above patch is a way forward but not the full fix. > > Thanks, > Andrew Pinski Andrew, are you still planning on submitting this patch? I have been running with your new "*mov<GPR:mode>_on_<GPR2:mode>_ne" instruction and that part at least works fine. I would like to get at least that much checked in for GCC 4.8. Steve Ellcey sellcey@mips.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 22:22 ` Andrew Pinski 2012-11-15 20:59 ` Richard Sandiford @ 2013-03-07 15:12 ` Jakub Jelinek 2013-03-07 16:01 ` Andrew Pinski 1 sibling, 1 reply; 14+ messages in thread From: Jakub Jelinek @ 2013-03-07 15:12 UTC (permalink / raw) To: Andrew Pinski; +Cc: Steve Ellcey, gcc-patches On Wed, Nov 14, 2012 at 02:22:33PM -0800, Andrew Pinski wrote: > commit 8ca1e58de404bbe82b93bc240ef28c68c681243d > Author: Andrew Pinski <apinski@cavium.com> > Date: Thu Jul 26 18:09:34 2012 -0700 > > 2012-07-26 Andrew Pinski <apinski@cavium.com> > > Bug #3261 > * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): > Remove mode check from comparisons. > (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. > (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match > when (ne A 0) can be just A. Why aren't you also adding a *mov<SCALARF:mode>_on_<GPR2:mode>_ne insn? Jakub ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2013-03-07 15:12 ` Jakub Jelinek @ 2013-03-07 16:01 ` Andrew Pinski 0 siblings, 0 replies; 14+ messages in thread From: Andrew Pinski @ 2013-03-07 16:01 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Steve Ellcey, gcc-patches On Thu, Mar 7, 2013 at 7:12 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 14, 2012 at 02:22:33PM -0800, Andrew Pinski wrote: >> commit 8ca1e58de404bbe82b93bc240ef28c68c681243d >> Author: Andrew Pinski <apinski@cavium.com> >> Date: Thu Jul 26 18:09:34 2012 -0700 >> >> 2012-07-26 Andrew Pinski <apinski@cavium.com> >> >> Bug #3261 >> * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): >> Remove mode check from comparisons. >> (*mov<SCALARF:mode>_on_<MOVECC:mode>): Likewise. >> (*mov<GPR:mode>_on_<GPR2:mode>_ne): New pattern to match >> when (ne A 0) can be just A. > > Why aren't you also adding a *mov<SCALARF:mode>_on_<GPR2:mode>_ne > insn? Most likely because I only tested performance of this patch on soft-float and I did not notice a reason for it yet. Thanks, Andrew Pinski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch] Performance patch for MIPS conditional move in expr.c 2012-11-14 19:02 [patch] Performance patch for MIPS conditional move in expr.c Steve Ellcey 2012-11-14 19:15 ` Andrew Pinski @ 2012-11-15 1:51 ` Richard Henderson 1 sibling, 0 replies; 14+ messages in thread From: Richard Henderson @ 2012-11-15 1:51 UTC (permalink / raw) To: Steve Ellcey; +Cc: gcc-patches, rguenther On 2012-11-14 11:02, Steve Ellcey wrote: > 2012-11-14 Steve Ellcey <sellcey@mips.com> > > * expr.c (expand_cond_expr_using_cmove): Use promoted mode for temp. Ok. r~ ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-03-07 16:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-14 19:02 [patch] Performance patch for MIPS conditional move in expr.c Steve Ellcey 2012-11-14 19:15 ` Andrew Pinski 2012-11-14 19:27 ` Steve Ellcey 2012-11-14 20:01 ` Andrew Pinski 2012-11-14 21:46 ` Steve Ellcey 2012-11-14 21:51 ` Andrew Pinski 2012-11-14 22:22 ` Andrew Pinski 2012-11-15 20:59 ` Richard Sandiford 2012-11-15 21:24 ` Andrew Pinski 2012-11-15 21:39 ` Andrew Pinski 2013-01-07 21:39 ` Steve Ellcey 2013-03-07 15:12 ` Jakub Jelinek 2013-03-07 16:01 ` Andrew Pinski 2012-11-15 1:51 ` Richard Henderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).