| Message ID | 27d19005-b82d-4ecc-a81e-14208937ce0f@EXCHHUB01.MIPS.com |
|---|---|
| State | New |
| Headers | show |
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,
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; }
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; > } >
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
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 > >
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~
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,