[patch] Performance patch for MIPS conditional move in expr.c

[patch] Performance patch for MIPS conditional move in expr.c

[Steve Ellcey]

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,

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

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,

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Steve Ellcey]

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;
}

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

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;
> }
>

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Steve Ellcey]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

[-- 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"

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

[-- 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;
+}
+

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Richard Henderson]

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~

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Richard Sandiford]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Steve Ellcey]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Jakub Jelinek]

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

Re: [patch] Performance patch for MIPS conditional move in expr.c

[Andrew Pinski]

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