public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [PATCH, MIPS] Add seq Octeon instruction
       [not found] ` <87ljye7rw2.fsf@firetop.home>
@ 2008-09-02 21:51   ` Adam Nemet
  2008-09-02 22:00     ` Adam Nemet
  2008-09-04 21:11   ` Adam Nemet
  1 sibling, 1 reply; 6+ messages in thread
From: Adam Nemet @ 2008-09-02 21:51 UTC (permalink / raw)
  To: Adam Nemet, gcc-patches, rdsandiford

Richard Sandiford wrote:
> I'm OK with this, but it's generally better to have a single
> matching pattern where possible.  Insns aren't necessarily
> rematched after reload, so it is technically possible for
> the new pattern to be used for zero comparisons.  E.g. we
> might foolishly have forced zero into a register, and then
> been unable to allocate that register.  Reload will then
> rewrite a two-register seq as an seq against zero.

Right, I should have remembered this.  Thanks.

> (define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
>   [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
> 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
> 		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
>   "ISA_HAS_SEQ_SNE"
>   "@
>     seq\t%0,%1,%2
>     sltiu\t%0,%1,1
>     seqi\t%0,%1,%2"

Did you intentionally separate the d and YB constraints into different
alternatives (I guess for better documentation) as opposed to just using
seq for both?   (gas can figure out which one to use just like for slt.)

> BTW, I'd missed the lack of "!TARGET_MIPS16" in the new ISA_HAS_* macros.
> Sorry about that.  Could you add them at some point?  (Doesn't matter
> whether it's part of another patch or separate.)  Preapproved, and doesn't
> need a full retest.
> 
> (I know it's daft testing !TARGET_MIPS16 on a processor that doesn't
> support MIPS16, but still, it is theoretically a supported combination.)

No it's fine, in fact, I already had this on my list of follow-on
patches.  My motivation was that if I miss NOMIPS16 from an octeon test
I'd like the test to fail with -mips16 rather then to mysteriously pass.

Adam

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, MIPS] Add seq Octeon instruction
  2008-09-02 21:51   ` [PATCH, MIPS] Add seq Octeon instruction Adam Nemet
@ 2008-09-02 22:00     ` Adam Nemet
  0 siblings, 0 replies; 6+ messages in thread
From: Adam Nemet @ 2008-09-02 22:00 UTC (permalink / raw)
  To: Adam Nemet, gcc-patches, rdsandiford

Adam Nemet wrote:
>> (define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
>>   [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
>> 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
>> 		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
>>   "ISA_HAS_SEQ_SNE"
>>   "@
>>     seq\t%0,%1,%2
>>     sltiu\t%0,%1,1
>>     seqi\t%0,%1,%2"
> 
> Did you intentionally separate the d and YB constraints into different
> alternatives (I guess for better documentation) as opposed to just using
> seq for both?   (gas can figure out which one to use just like for slt.)

Never mind, I understand now :).

Adam

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, MIPS] Add seq Octeon instruction
       [not found] ` <87ljye7rw2.fsf@firetop.home>
  2008-09-02 21:51   ` [PATCH, MIPS] Add seq Octeon instruction Adam Nemet
@ 2008-09-04 21:11   ` Adam Nemet
  2008-09-04 21:27     ` Richard Sandiford
  1 sibling, 1 reply; 6+ messages in thread
From: Adam Nemet @ 2008-09-04 21:11 UTC (permalink / raw)
  To: Adam Nemet, gcc-patches, rdsandiford

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

Richard Sandiford wrote:
> So if we want to use sltiu for zero comparisons, I think
> it would be technically better to have something like:
> 
> (define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
>   [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
> 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
> 		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
>   "ISA_HAS_SEQ_SNE"
>   "@
>     seq\t%0,%1,%2
>     sltiu\t%0,%1,1
>     seqi\t%0,%1,%2"
>   [(set_attr "type" "slt")
>    (set_attr "mode" "<GPR:MODE>")])

Here it is.  Bootstrapped and tested on mips64octeon-unknown-linux-gnu.

I had to slightly modify the tests (even existing ones) to accept both
sltu and sltiu in the assembly.

Looks good?

Adam


[-- Attachment #2: seq-2.patch --]
[-- Type: text/x-patch, Size: 9273 bytes --]

	* config/mips/mips.h (ISA_HAS_SEQ_SNE): New macro.
	* config/mips/mips.c (mips_expand_scc): Also expand seq and sne if
	second operand is a reg_imm10_operand.
	* config/mips/mips.md (*seq_<GPR:mode><GPR2:mode>_seq,
	*sne_<GPR:mode><GPR2:mode>_sne): New patterns.
	(*seq_<GPR:mode><GPR2:mode>): Rename to
	*seq_zero_<GPR:mode><GPR2:mode>.  Don't match if
	ISA_HAS_SEQ_SNE.
	(*seq_<GPR:mode><GPR2:mode>_mips16): Rename to
	*seq_zero_<GPR:mode><GPR2:mode>_mip16.  Don't match if
	ISA_HAS_SEQ_SNE.
	(*sne_<GPR:mode><GPR2:mode>): Rename to
	*sne_zero_<GPR:mode><GPR2:mode>.  Don't match if
	ISA_HAS_SEQ_SNE.

testsuite/
	* gcc.target/mips/seq-1.c: New test.
	* gcc.target/mips/octeon-seq-1.c: New test.
	* gcc.target/mips/octeon-seq-2.c: New test.
	* gcc.target/mips/octeon-seq-3.c: New test.
	* gcc.target/mips/octeon-seq-4.c: New test.
	* gcc.target/mips/scc-2.c: Also pass on sltiu.
	* gcc.target/mips/scc-3.c: Likewise.

Index: gcc/config/mips/mips.h
===================================================================
--- gcc.orig/config/mips/mips.h	2008-09-02 15:20:11.000000000 -0700
+++ gcc/config/mips/mips.h	2008-09-03 17:24:27.000000000 -0700
@@ -1015,6 +1015,9 @@ enum mips_code_readable_setting {
 /* ISA includes the exts instruction.  */
 #define ISA_HAS_EXTS		TARGET_OCTEON
 
+/* ISA includes the seq and sne instructions.  */
+#define ISA_HAS_SEQ_SNE		TARGET_OCTEON
+
 /* ISA includes the pop instruction.  */
 #define ISA_HAS_POP		TARGET_OCTEON
 \f
Index: gcc/config/mips/mips.c
===================================================================
--- gcc.orig/config/mips/mips.c	2008-09-02 15:20:05.000000000 -0700
+++ gcc/config/mips/mips.c	2008-09-03 17:24:27.000000000 -0700
@@ -4222,8 +4222,14 @@ mips_expand_scc (enum rtx_code code, rtx
 
   if (code == EQ || code == NE)
     {
-      rtx zie = mips_zero_if_equal (cmp_operands[0], cmp_operands[1]);
-      mips_emit_binary (code, target, zie, const0_rtx);
+      if (ISA_HAS_SEQ_SNE
+	  && reg_imm10_operand (cmp_operands[1], GET_MODE (cmp_operands[1])))
+	mips_emit_binary (code, target, cmp_operands[0], cmp_operands[1]);
+      else
+	{
+	  rtx zie = mips_zero_if_equal (cmp_operands[0], cmp_operands[1]);
+	  mips_emit_binary (code, target, zie, const0_rtx);
+	}
     }
   else
     mips_emit_int_order_test (code, 0, target,
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md	2008-09-02 15:20:11.000000000 -0700
+++ gcc/config/mips/mips.md	2008-09-03 17:24:27.000000000 -0700
@@ -5200,24 +5200,36 @@
   ""
   { if (mips_expand_scc (EQ, operands[0])) DONE; else FAIL; })
 
-(define_insn "*seq_<GPR:mode><GPR2:mode>"
+(define_insn "*seq_zero_<GPR:mode><GPR2:mode>"
   [(set (match_operand:GPR2 0 "register_operand" "=d")
 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "d")
 		 (const_int 0)))]
-  "!TARGET_MIPS16"
+  "!TARGET_MIPS16 && !ISA_HAS_SEQ_SNE"
   "sltu\t%0,%1,1"
   [(set_attr "type" "slt")
    (set_attr "mode" "<GPR:MODE>")])
 
-(define_insn "*seq_<GPR:mode><GPR2:mode>_mips16"
+(define_insn "*seq_zero_<GPR:mode><GPR2:mode>_mips16"
   [(set (match_operand:GPR2 0 "register_operand" "=t")
 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "d")
 		 (const_int 0)))]
-  "TARGET_MIPS16"
+  "TARGET_MIPS16 && !ISA_HAS_SEQ_SNE"
   "sltu\t%1,1"
   [(set_attr "type" "slt")
    (set_attr "mode" "<GPR:MODE>")])
 
+(define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
+  [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
+	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
+		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
+  "ISA_HAS_SEQ_SNE"
+  "@
+   seq\t%0,%1,%2
+   sltiu\t%0,%1,1
+   seqi\t%0,%1,%2"
+  [(set_attr "type" "slt")
+   (set_attr "mode" "<GPR:MODE>")])
+
 ;; "sne" uses sltu instructions in which the first operand is $0.
 ;; This isn't possible in mips16 code.
 
@@ -5228,15 +5240,27 @@
   "!TARGET_MIPS16"
   { if (mips_expand_scc (NE, operands[0])) DONE; else FAIL; })
 
-(define_insn "*sne_<GPR:mode><GPR2:mode>"
+(define_insn "*sne_zero_<GPR:mode><GPR2:mode>"
   [(set (match_operand:GPR2 0 "register_operand" "=d")
 	(ne:GPR2 (match_operand:GPR 1 "register_operand" "d")
 		 (const_int 0)))]
-  "!TARGET_MIPS16"
+  "!TARGET_MIPS16 && !ISA_HAS_SEQ_SNE"
   "sltu\t%0,%.,%1"
   [(set_attr "type" "slt")
    (set_attr "mode" "<GPR:MODE>")])
 
+(define_insn "*sne_<GPR:mode><GPR2:mode>_sne"
+  [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
+	(ne:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
+		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
+  "ISA_HAS_SEQ_SNE"
+  "@
+   sne\t%0,%1,%2
+   sltu\t%0,%.,%1
+   snei\t%0,%1,%2"
+  [(set_attr "type" "slt")
+   (set_attr "mode" "<GPR:MODE>")])
+
 (define_expand "sgt<u>"
   [(set (match_operand:SI 0 "register_operand")
 	(any_gt:SI (match_dup 1)
Index: gcc/testsuite/gcc.target/mips/seq-1.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/seq-1.c	2008-09-02 15:59:03.000000000 -0700
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler-times "\tsltu\t|\tsltiu\t" 4 } } */
+
+#define TEST(N, LHS, REL, RHS) \
+  NOMIPS16 int f##N (int a, int b) { return LHS REL RHS; }
+
+TEST (0, a, ==, 0);
+TEST (1, a, ==, 600);
+TEST (10, a, !=, 0);
+TEST (11, a, !=, -800);
Index: gcc/testsuite/gcc.target/mips/octeon-seq-1.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-seq-1.c	2008-09-02 15:52:50.000000000 -0700
@@ -0,0 +1,19 @@
+/* Check if we expand seq and sne.  */
+
+/* { dg-do compile } */
+/* { dg-mips-options "-march=octeon" } */
+/* { dg-final { scan-assembler-times "\tseq\t|\tseqi\t" 4 } } */
+/* { dg-final { scan-assembler-times "\tsne\t|\tsnei\t" 4 } } */
+
+#define TEST(N, LHS, REL, RHS) \
+  NOMIPS16 int f##N (int a, int b) { return LHS REL RHS; }
+
+TEST (0, a, ==, b);
+TEST (1, a, ==, 23);
+TEST (2, a, ==, 511);
+TEST (3, a, ==, -200);
+
+TEST (10, a, !=, b);
+TEST (11, a, !=, 1);
+TEST (12, a, !=, 350);
+TEST (13, a, !=, -512);
Index: gcc/testsuite/gcc.target/mips/octeon-seq-2.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-seq-2.c	2008-09-02 15:53:26.000000000 -0700
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-march=octeon -mgp64" } */
+/* { dg-final { scan-assembler-times "\tseq\t|\tseqi\t" 3 } } */
+/* { dg-final { scan-assembler-times "\tsne\t|\tsnei\t" 3 } } */
+
+#define TEST(N, LHS, REL, RHS) \
+  NOMIPS16 long long f##N (long long a, long long b) { return LHS REL RHS; }
+
+TEST (0, a, ==, b);
+TEST (1, a, ==, 23);
+TEST (2, a, ==, 511);
+
+TEST (3, a, !=, b);
+TEST (4, a, !=, 1);
+TEST (5, a, !=, 350);
Index: gcc/testsuite/gcc.target/mips/octeon-seq-3.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-seq-3.c	2008-09-02 15:51:24.000000000 -0700
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O -march=octeon -mgp64" } */
+
+/* { dg-final { scan-assembler-not "and\t\|andi\t\|ext\t\|sll\t\|srl\t" } } */
+/* { dg-final { scan-assembler-times "\tseqi\t\|\tsnei\t" 4 } } */
+
+
+#define TEST(N, LHS, REL, RHS) \
+  NOMIPS16 long long w##N (int a, int b) {return LHS REL RHS;} \
+  NOMIPS16 int n##N (long long a, long long b) {return LHS REL RHS;} \
+
+TEST (eq, a, ==, 10);
+TEST (ne, a, !=, 32);
Index: gcc/testsuite/gcc.target/mips/octeon-seq-4.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-seq-4.c	2008-09-02 15:20:12.000000000 -0700
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-final { scan-assembler-not "xor" } } */
+
+unsigned
+m (unsigned e);
+
+NOMIPS16 void
+f (unsigned i)
+{
+  unsigned j = m (i);
+  h (j, i != j);
+}
Index: gcc/testsuite/gcc.target/mips/scc-2.c
===================================================================
--- gcc.orig/testsuite/gcc.target/mips/scc-2.c	2008-09-03 18:42:34.000000000 -0700
+++ gcc/testsuite/gcc.target/mips/scc-2.c	2008-09-03 18:47:15.000000000 -0700
@@ -2,7 +2,7 @@
 /* { dg-mips-options "-O -mgp64" } */
 
 /* { dg-final { scan-assembler-not "and\t\|andi\t\|ext\t\|sll\t\|srl\t" } } */
-/* { dg-final { scan-assembler-times "slt\t\|sltu\t" 12 } } */
+/* { dg-final { scan-assembler-times "slt\t\|slti?u\t" 12 } } */
 
 
 #define TEST(N, LHS, REL, RHS) \
Index: gcc/testsuite/gcc.target/mips/scc-3.c
===================================================================
--- gcc.orig/testsuite/gcc.target/mips/scc-3.c	2008-09-03 18:44:46.000000000 -0700
+++ gcc/testsuite/gcc.target/mips/scc-3.c	2008-09-03 18:46:59.000000000 -0700
@@ -3,7 +3,7 @@
 /* { dg-add-options mips16_attribute } */
 
 /* { dg-final { scan-assembler-not "and\t\|andi\t\|ext\t\|sll\t\|srl\t" } } */
-/* { dg-final { scan-assembler-times "slt\t\|sltu\t" 8 } } */
+/* { dg-final { scan-assembler-times "slt\t\|slti?u\t" 8 } } */
 
 
 #define TEST(N, LHS, REL, RHS) \

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, MIPS] Add seq Octeon instruction
  2008-09-04 21:11   ` Adam Nemet
@ 2008-09-04 21:27     ` Richard Sandiford
  2008-09-04 21:47       ` Adam Nemet
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2008-09-04 21:27 UTC (permalink / raw)
  To: Adam Nemet; +Cc: gcc-patches

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> So if we want to use sltiu for zero comparisons, I think
>> it would be technically better to have something like:
>> 
>> (define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
>>   [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
>> 	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
>> 		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
>>   "ISA_HAS_SEQ_SNE"
>>   "@
>>     seq\t%0,%1,%2
>>     sltiu\t%0,%1,1
>>     seqi\t%0,%1,%2"
>>   [(set_attr "type" "slt")
>>    (set_attr "mode" "<GPR:MODE>")])
>
> Here it is.  Bootstrapped and tested on mips64octeon-unknown-linux-gnu.
>
> I had to slightly modify the tests (even existing ones) to accept both
> sltu and sltiu in the assembly.
>
> Looks good?

Yes, thanks.  OK for trunk.

TBH, I'm curious why it's better to use sltiu over the new instructions
for the zero case.  Is it just tradition?  Or is there some performance
benefit?  The patch is OK regardless, but if there's something you could
add as a comment, that'd be great.

Richard

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, MIPS] Add seq Octeon instruction
  2008-09-04 21:27     ` Richard Sandiford
@ 2008-09-04 21:47       ` Adam Nemet
  2008-09-04 22:07         ` Richard Sandiford
  0 siblings, 1 reply; 6+ messages in thread
From: Adam Nemet @ 2008-09-04 21:47 UTC (permalink / raw)
  To: Adam Nemet, gcc-patches, rdsandiford

Richard Sandiford wrote:
> Yes, thanks.  OK for trunk.

Thanks.

> TBH, I'm curious why it's better to use sltiu over the new instructions
> for the zero case.  Is it just tradition?  Or is there some performance
> benefit?  The patch is OK regardless, but if there's something you could
> add as a comment, that'd be great.

Yeah, just tradition.  It's been useful in the past to count how many
times our instructions are used over standard MIPS (and this is of
course only valid if we generate MIPS when the two are interchangeable).

Do you still want me to add a comment like this:

;; Generate slt unless using seq/sne results in better code.

Adam

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, MIPS] Add seq Octeon instruction
  2008-09-04 21:47       ` Adam Nemet
@ 2008-09-04 22:07         ` Richard Sandiford
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Sandiford @ 2008-09-04 22:07 UTC (permalink / raw)
  To: Adam Nemet; +Cc: gcc-patches

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> Yes, thanks.  OK for trunk.
>
> Thanks.
>
>> TBH, I'm curious why it's better to use sltiu over the new instructions
>> for the zero case.  Is it just tradition?  Or is there some performance
>> benefit?  The patch is OK regardless, but if there's something you could
>> add as a comment, that'd be great.
>
> Yeah, just tradition.  It's been useful in the past to count how many
> times our instructions are used over standard MIPS (and this is of
> course only valid if we generate MIPS when the two are interchangeable).

Ah, OK.

> Do you still want me to add a comment like this:
>
> ;; Generate slt unless using seq/sne results in better code.

Yeah, sounds good.

Richard

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-09-04 22:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <48B8AD98.60507@caviumnetworks.com>
     [not found] ` <87ljye7rw2.fsf@firetop.home>
2008-09-02 21:51   ` [PATCH, MIPS] Add seq Octeon instruction Adam Nemet
2008-09-02 22:00     ` Adam Nemet
2008-09-04 21:11   ` Adam Nemet
2008-09-04 21:27     ` Richard Sandiford
2008-09-04 21:47       ` Adam Nemet
2008-09-04 22:07         ` Richard Sandiford

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).