diff mbox

Performance patch for MIPS conditional move in expr.c

Message ID CA+=Sn1nNxA+D1GQAbG84YWKmZc8eu=MbD91aiJHTmy2gT4S_Vw@mail.gmail.com
State New
Headers show

Commit Message

Andrew Pinski Nov. 14, 2012, 10:22 p.m. UTC 
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
>>
>>
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.

Comments

Richard Sandiford Nov. 15, 2012, 8:58 p.m. UTC | #1 
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
Andrew Pinski Nov. 15, 2012, 9:24 p.m. UTC | #2 
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
Andrew Pinski Nov. 15, 2012, 9:39 p.m. UTC | #3 
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
Steve Ellcey Jan. 7, 2013, 9:38 p.m. UTC | #4 
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
Jakub Jelinek March 7, 2013, 3:12 p.m. UTC | #5 
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
Andrew Pinski March 7, 2013, 4 p.m. UTC | #6 
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
diff mbox

Patch

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