Patchwork Performance patch for MIPS conditional move in expr.c

login
register
mail settings
Submitter Steve Ellcey
Date Nov. 14, 2012, 7:02 p.m.
Message ID <27d19005-b82d-4ecc-a81e-14208937ce0f@EXCHHUB01.MIPS.com>
Download mbox | patch
Permalink /patch/198980/
State New
Headers show

Comments

Steve Ellcey - Nov. 14, 2012, 7:02 p.m.
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.
Andrew Pinski - Nov. 14, 2012, 7:15 p.m.
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,
Steve Ellcey - Nov. 14, 2012, 7:27 p.m.
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;
}
Andrew Pinski - Nov. 14, 2012, 8 p.m.
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;
> }
>
Steve Ellcey - Nov. 14, 2012, 9:45 p.m.
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
Andrew Pinski - Nov. 14, 2012, 9:51 p.m.
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
>
>
Richard Henderson - Nov. 14, 2012, 10:07 p.m.
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~

Patch

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,