[PATCH, MIPS] Add bbit* Octeon instructions

[PATCH, MIPS] Add bbit* Octeon instructions

[Adam Nemet]

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

This adds support for the bbit[01] instructions.  The bbit[01]
instructions branch depending on the value of a bit in an integer
register.  For more precise definition as with all other Octeon
instructions please see the IS manual
(http://www.cnusers.org/index.php?option=com_remository&Itemid=32&func=startdown&id=48).

The new patterns combine a single-bit zero extract with an equality
branch.  The canonical forms for these always compare against zero with
the difference that bbit0 does equality comparison while bbit1 does
non-equality.  That makes these new patterns similar to the standard
branch-on-equality patterns.

I still decided against using the match_operator approach.  IMO, the
same thing with code iterators is much more readable, mostly because
there is no need to use cryptic operand prefixes.  It might even be more
efficient than calling the predicate inside match_operator.  I think we
should try changing the equality branch along the same lines.

For branch inversion I just added a separate pattern.  I can merge the
two along the idea in the mips16 equality pattern if that's preferred.
Or we can just convert all of them at once.

According to md.texi the first argument of extzv is always in word_mode.
 Therefore in 64-bit mode we should only have DI patterns for extzv.  We
have both SI and DI patterns so to match that I added branch_bit to also
have an SI-mode pattern in 64-bit mode.

I am planning to revisit the extzv 64r2 patterns because apparently
there are other issues as well.  Right now we miscompile this:

  struct s
  {
    unsigned long long a:16;
    unsigned long long b:32;
    unsigned long long c:16;
  };

  unsigned
  f (struct s s)
  {
    return s.b;  // dext, should be ext or dext;sll,,0
  }

We generate a dext here without later truncating the value.

If that's acceptable I'd like to sort out whether we need the SI-mode
extraction while fixing this miscompilation.  And then depending on the
outcome also remove SI mode from the bbit pattern.

The bbit* instructions have no likely variants.  I added a new
define_delay definition where we don't annul on false.  This is all
driven by the new insn attribute has_likely.  octeon-bbit-2.c is a
testcase for the change.

Bootstrapped and tested on mips64octeon-unknown-linux-gnu.  The new
tests work both in 64-bit and in 32-bit mode.

OK to install?

Adam

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

	* config/mips/mips.h (ISA_HAS_BBIT): New macro.
	* config/mips/mips.md (has_likely): New attribute.  Default to
	yes.
	(define_delay for type "branch"): Change to not apply for branch
	without likely variant.
	(define_delay for type "branch" and "has_likely" no).  New delay
	definition.
	(equality_op): New code iterator.
	(bbv, bbinv): New code attributes.
	(*branch_bit<bbv><mode>, *branch_bit<bbv><mode>_inverted): New
	patterns.

testsuite/
	* gcc.target/mips/octeon-bbit-1.c: New test.
	* gcc.target/mips/octeon-bbit-2.c: New test.
	* gcc.target/mips/octeon-bbit-3.c: New test.

Index: gcc/config/mips/mips.h
===================================================================
--- gcc.orig/config/mips/mips.h	2008-08-25 16:03:04.000000000 -0700
+++ gcc/config/mips/mips.h	2008-08-25 16:14:37.000000000 -0700
@@ -1008,6 +1008,9 @@ enum mips_code_readable_setting {
 
 /* ISA includes the baddu instruction.  */
 #define ISA_HAS_BADDU		TARGET_OCTEON
+
+/* ISA includes the bbit* instructions.  */
+#define ISA_HAS_BBIT		TARGET_OCTEON
 \f
 /* Add -G xx support.  */
 
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md	2008-08-25 16:03:04.000000000 -0700
+++ gcc/config/mips/mips.md	2008-08-25 18:28:59.000000000 -0700
@@ -613,6 +613,10 @@
 		(const_string "yes")
 		(const_string "no")))
 
+;; True for a branch instruction if it has a branch-likely variant.
+(define_attr "has_likely" "no,yes"
+  (const_string "yes"))
+
 ;; Describe a user's asm statement.
 (define_asm_attributes
   [(set_attr "type" "multi")
@@ -781,6 +785,9 @@
 ;; by swapping the operands.
 (define_code_iterator swapped_fcond [ge gt unge ungt])
 
+;; Equality operators.
+(define_code_iterator equality_op [eq ne])
+
 ;; These code iterators allow the signed and unsigned scc operations to use
 ;; the same template.
 (define_code_iterator any_gt [gt gtu])
@@ -844,6 +851,13 @@
 
 ;; Atomic HI and QI operations
 (define_code_iterator atomic_hiqi_op [plus minus ior xor and])
+
+;; The value of the bit when the branch is taken for branch_bit patterns.
+;; Comparison is always against zero so this depends on the operator.
+(define_code_attr bbv [(eq "0") (ne "1")])
+
+;; This is the inverse value of bbv.
+(define_code_attr bbinv [(eq "1") (ne "0")])
 \f
 ;; .........................
 ;;
@@ -852,12 +866,21 @@
 ;; .........................
 
 (define_delay (and (eq_attr "type" "branch")
-		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
+		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
+			(eq_attr "has_likely" "yes")))
   [(eq_attr "can_delay" "yes")
    (nil)
    (and (eq_attr "branch_likely" "yes")
 	(eq_attr "can_delay" "yes"))])
 
+;; Branches that don't have likely variants do not annul on false.
+(define_delay (and (eq_attr "type" "branch")
+		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
+			(eq_attr "has_likely" "no")))
+  [(eq_attr "can_delay" "yes")
+   (nil)
+   (nil)])
+
 (define_delay (eq_attr "type" "jump")
   [(eq_attr "can_delay" "yes")
    (nil)
@@ -5574,6 +5597,50 @@
 	(if_then_else (match_operand 0)
 		      (label_ref (match_operand 1))
 		      (pc)))])
+
+;; Branch if bit is set/clear.
+
+(define_insn "*branch_bit<bbv><mode>"
+  [(set (pc)
+	(if_then_else
+	 (equality_op (zero_extract:GPR
+		       (match_operand:GPR 1 "register_operand" "d")
+		       (const_int 1)
+		       (match_operand 2 "immediate_operand" "I"))
+		      (const_int 0))
+	 (label_ref (match_operand 0 ""))
+	 (pc)))]
+  "ISA_HAS_BBIT"
+{
+  return
+    mips_output_conditional_branch (insn, operands,
+				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"),
+				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"));
+}
+  [(set_attr "type"	  "branch")
+   (set_attr "mode"	  "none")
+   (set_attr "has_likely" "no")])
+
+(define_insn "*branch_bit<bbv><mode>_inverted"
+  [(set (pc)
+	(if_then_else
+	 (equality_op (zero_extract:GPR
+		       (match_operand:GPR 1 "register_operand" "d")
+		       (const_int 1)
+		       (match_operand 2 "immediate_operand" "I"))
+		      (const_int 0))
+	 (pc)
+	 (label_ref (match_operand 0 ""))))]
+  "ISA_HAS_BBIT"
+{
+  return
+    mips_output_conditional_branch (insn, operands,
+				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"),
+				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"));
+}
+  [(set_attr "type"	  "branch")
+   (set_attr "mode"	  "none")
+   (set_attr "has_likely" "no")])
 \f
 ;;
 ;;  ....................
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-1.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-1.c	2008-08-25 16:14:59.000000000 -0700
@@ -0,0 +1,55 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-final { scan-assembler-times "\tbbit1\t" 4 } } */
+/* { dg-final { scan-assembler-times "\tbbit0\t" 2 } } */
+/* { dg-final { scan-assembler-not "andi\t" } } */
+
+NOMIPS16 void
+f1 (long long i)
+{
+  if (i & 0x80)
+    foo ();
+}
+
+NOMIPS16 void
+f2 (int i)
+{
+  if (!(i & 0x80))
+    foo ();
+}
+
+NOMIPS16 void
+f3 (int i)
+{
+  if (i % 2)
+    foo ();
+}
+
+NOMIPS16 void
+f4 (int i)
+{
+  if (i & 1)
+    foo ();
+}
+
+NOMIPS16 void
+f5 (long long i)
+{
+  if ((i >> 3) & 1)
+    foo ();
+}
+
+unsigned long long r;
+
+NOMIPS16 static inline __attribute__((always_inline)) int
+test_bit(unsigned long long nr, const unsigned long long *addr)
+{
+  return 1UL & (addr[nr >> 6] >> (nr & 63ULL));
+}
+
+NOMIPS16 void
+f6 ()
+{
+  if (!test_bit(0, &r))
+    g ();
+}
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-2.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-2.c	2008-08-25 16:14:37.000000000 -0700
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon -mbranch-likely" } */
+/* { dg-final { scan-assembler "\tbbit\[01\]\t" } } */
+/* { dg-final { scan-assembler-not "\tbbit\[01\]l\t" } } */
+/* { dg-final { scan-assembler "\tbnel\t" } } */
+/* { dg-final { scan-assembler-not "\tbne\t" } } */
+
+NOMIPS16 int
+f (int n, int i)
+{
+  int s = 0;
+  for (; i & 1; i++)
+    s += i;
+  return s;
+}
+
+NOMIPS16 int
+g (int n, int i)
+{
+  int s = 0;
+  for (i = 0; i < n; i++)
+    s += i;
+  return s;
+}
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-3.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-3.c	2008-08-25 16:34:59.000000000 -0700
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
+/* { dg-final { scan-assembler-not "ext\t" } } */
+
+void abort (void);
+void exit (int);
+
+typedef unsigned long long ulong64;
+
+typedef struct bitfield_s {
+  ulong64 a:1;
+  ulong64 b:29;
+  ulong64 c:1;
+  ulong64 d:15;
+  ulong64 f:18;
+} bitfield_t;
+
+bitfield_t bar;
+
+NOMIPS16 void
+f ()
+{
+  foo(&bar);
+  if (bar.a != 0x1)
+    abort ();
+  else if (!bar.c)
+    abort ();
+  else
+    exit (0);
+}

Re: [PATCH, MIPS] Add bbit* Octeon instructions

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> I still decided against using the match_operator approach.  IMO, the
> same thing with code iterators is much more readable, mostly because
> there is no need to use cryptic operand prefixes.  It might even be more
> efficient than calling the predicate inside match_operator.  I think we
> should try changing the equality branch along the same lines.

OK.

> For branch inversion I just added a separate pattern.  I can merge the
> two along the idea in the mips16 equality pattern if that's preferred.
> Or we can just convert all of them at once.

For the record, I personally don't prefer the MIPS16 approach.
I've always been a bit uncomfortable with the lack of a "one operand
must be (pc)" check, even though gcc doesn't really support two-label
branches at the moment.

> According to md.texi the first argument of extzv is always in word_mode.
>  Therefore in 64-bit mode we should only have DI patterns for extzv.  We
> have both SI and DI patterns so to match that I added branch_bit to also
> have an SI-mode pattern in 64-bit mode.

The documentation refers to the extzv expander.  I believe the argument to
that expander is as described, which means that the extracted value is often
a 64-bit paradoxical subreg of a 32-bit value.  But my understanding is
that it's OK for backends to generate zero_extracts with smaller modes.
We explicitly strip the subreg for unaligned loads and stores, but it
doesn't look we do for zero_extracts themselves.  I imagine it can still
get (legitimately) matched by combine though.

Regardless, I think using GPR here (as you did) is the right thing to do.

> I am planning to revisit the extzv 64r2 patterns because apparently
> there are other issues as well.  Right now we miscompile this:
>
>   struct s
>   {
>     unsigned long long a:16;
>     unsigned long long b:32;
>     unsigned long long c:16;
>   };
>
>   unsigned
>   f (struct s s)
>   {
>     return s.b;  // dext, should be ext or dext;sll,,0
>   }
>
> We generate a dext here without later truncating the value.
>
> If that's acceptable I'd like to sort out whether we need the SI-mode
> extraction while fixing this miscompilation.  And then depending on the
> outcome also remove SI mode from the bbit pattern.

Won't you still need SImode for 32-bit targets?  (I realise you're
planning on removing the o32 multilib, but still... it seems odd
remove SImode stuff from 64-bit targets.)

> @@ -852,12 +866,21 @@
>  ;; .........................
>  
>  (define_delay (and (eq_attr "type" "branch")
> -		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "yes")))
>    [(eq_attr "can_delay" "yes")
>     (nil)
>     (and (eq_attr "branch_likely" "yes")
>  	(eq_attr "can_delay" "yes"))])
>  
> +;; Branches that don't have likely variants do not annul on false.
> +(define_delay (and (eq_attr "type" "branch")
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "no")))
> +  [(eq_attr "can_delay" "yes")
> +   (nil)
> +   (nil)])
> +

Can't you simply use:

  (set_attr "branch_likely" "no")

avoiding the extra define_delay and "has_likely" attribute?

>  (define_delay (eq_attr "type" "jump")
>    [(eq_attr "can_delay" "yes")
>     (nil)
> @@ -5574,6 +5597,50 @@
>  	(if_then_else (match_operand 0)
>  		      (label_ref (match_operand 1))
>  		      (pc)))])
> +
> +;; Branch if bit is set/clear.
> +
> +(define_insn "*branch_bit<bbv><mode>"
> +  [(set (pc)
> +	(if_then_else
> +	 (equality_op (zero_extract:GPR
> +		       (match_operand:GPR 1 "register_operand" "d")
> +		       (const_int 1)
> +		       (match_operand 2 "immediate_operand" "I"))
> +		      (const_int 0))
> +	 (label_ref (match_operand 0 ""))
> +	 (pc)))]
> +  "ISA_HAS_BBIT"
> +{
> +  return
> +    mips_output_conditional_branch (insn, operands,
> +				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"),
> +				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"));
> +}
> +  [(set_attr "type"	  "branch")
> +   (set_attr "mode"	  "none")
> +   (set_attr "has_likely" "no")])

I'd prefer "const_int_operand" "" over "immediate_operand" "I".
(The second version probably still appears in mips.md, but it's
technically bad to have predicates that only match constants,
and whose constraints disallow things that the predicates allow.)

I don't know whether a range check is needed here or not, but since
this area seems a little under-specified, it might be safer to add one:

  "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<GPR>mode)"

OK with those changes, thanks.

Richard

Re: [PATCH, MIPS] Add bbit* Octeon instructions

[Adam Nemet]

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

Richard Sandiford wrote:
> Adam Nemet <anemet@caviumnetworks.com> writes:
>> If that's acceptable I'd like to sort out whether we need the SI-mode
>> extraction while fixing this miscompilation.  And then depending on the
>> outcome also remove SI mode from the bbit pattern.
> 
> Won't you still need SImode for 32-bit targets?  (I realise you're
> planning on removing the o32 multilib, but still... it seems odd
> remove SImode stuff from 64-bit targets.)

What I meant was to remove the SI pattern for TARGET_64BIT not for
!TARGET_64BIT (o32).  Basically writing these patterns with a new mode
iterator:

  (define_mode_iterator WORD [(SI "!TARGET_64BIT") (DI "TARGET_64BIT)])

(In fact I had a parallel version of this patch written and tested with
WORD and the bbit tests were passing with o32 and I had the same number
of bbit instructions in libgcj.so.)

>> @@ -852,12 +866,21 @@
>>  ;; .........................
>>  
>>  (define_delay (and (eq_attr "type" "branch")
>> -		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
>> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
>> +			(eq_attr "has_likely" "yes")))
>>    [(eq_attr "can_delay" "yes")
>>     (nil)
>>     (and (eq_attr "branch_likely" "yes")
>>  	(eq_attr "can_delay" "yes"))])
>>  
>> +;; Branches that don't have likely variants do not annul on false.
>> +(define_delay (and (eq_attr "type" "branch")
>> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
>> +			(eq_attr "has_likely" "no")))
>> +  [(eq_attr "can_delay" "yes")
>> +   (nil)
>> +   (nil)])
>> +
> 
> Can't you simply use:
> 
>   (set_attr "branch_likely" "no")
> 
> avoiding the extra define_delay and "has_likely" attribute?

No and I should have really explained that in the email because this had
been my first reaction as well.  The annul predicates test the
*candidate instruction* for the delay slot and *not* the branch
instruction.  Only the actual define_delay predicate is a test on the
branch instruction so the check needs to go there.

> I'd prefer "const_int_operand" "" over "immediate_operand" "I".
> (The second version probably still appears in mips.md, but it's
> technically bad to have predicates that only match constants,
> and whose constraints disallow things that the predicates allow.)

I see.  Thanks for the explanation.  I made the change.

> I don't know whether a range check is needed here or not, but since
> this area seems a little under-specified, it might be safer to add one:
> 
>   "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<GPR>mode)"

I added that too, can't hurt.

> OK with those changes, thanks.

Let me know if you agree with the has_likely explanation.  Attached is
what I have successfully rebootstrapped and retested.

Adam


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

	* config/mips/mips.h (ISA_HAS_BBIT): New macro.
	* config/mips/mips.md (has_likely): New attribute.  Default to
	yes.
	(define_delay for type "branch"): Change to not apply for branch
	without likely variant.
	(define_delay for type "branch" and "has_likely" no).  New delay
	definition.
	(equality_op): New code iterator.
	(bbv, bbinv): New code attributes.
	(*branch_bit<bbv><mode>, *branch_bit<bbv><mode>_inverted): New
	patterns.

testsuite/
	* gcc.target/mips/octeon-bbit-1.c: New test.
	* gcc.target/mips/octeon-bbit-2.c: New test.
	* gcc.target/mips/octeon-bbit-3.c: New test.

Index: gcc/config/mips/mips.h
===================================================================
--- gcc.orig/config/mips/mips.h	2008-08-26 15:07:56.000000000 -0700
+++ gcc/config/mips/mips.h	2008-08-26 17:29:49.000000000 -0700
@@ -1008,6 +1008,9 @@ enum mips_code_readable_setting {
 
 /* ISA includes the baddu instruction.  */
 #define ISA_HAS_BADDU		TARGET_OCTEON
+
+/* ISA includes the bbit* instructions.  */
+#define ISA_HAS_BBIT		TARGET_OCTEON
 \f
 /* Add -G xx support.  */
 
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md	2008-08-26 16:55:17.000000000 -0700
+++ gcc/config/mips/mips.md	2008-08-26 17:37:20.000000000 -0700
@@ -613,6 +613,10 @@
 		(const_string "yes")
 		(const_string "no")))
 
+;; True for a branch instruction if it has a branch-likely variant.
+(define_attr "has_likely" "no,yes"
+  (const_string "yes"))
+
 ;; Describe a user's asm statement.
 (define_asm_attributes
   [(set_attr "type" "multi")
@@ -781,6 +785,9 @@
 ;; by swapping the operands.
 (define_code_iterator swapped_fcond [ge gt unge ungt])
 
+;; Equality operators.
+(define_code_iterator equality_op [eq ne])
+
 ;; These code iterators allow the signed and unsigned scc operations to use
 ;; the same template.
 (define_code_iterator any_gt [gt gtu])
@@ -844,6 +851,13 @@
 
 ;; Atomic HI and QI operations
 (define_code_iterator atomic_hiqi_op [plus minus ior xor and])
+
+;; The value of the bit when the branch is taken for branch_bit patterns.
+;; Comparison is always against zero so this depends on the operator.
+(define_code_attr bbv [(eq "0") (ne "1")])
+
+;; This is the inverse value of bbv.
+(define_code_attr bbinv [(eq "1") (ne "0")])
 \f
 ;; .........................
 ;;
@@ -852,12 +866,21 @@
 ;; .........................
 
 (define_delay (and (eq_attr "type" "branch")
-		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
+		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
+			(eq_attr "has_likely" "yes")))
   [(eq_attr "can_delay" "yes")
    (nil)
    (and (eq_attr "branch_likely" "yes")
 	(eq_attr "can_delay" "yes"))])
 
+;; Branches that don't have likely variants do not annul on false.
+(define_delay (and (eq_attr "type" "branch")
+		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
+			(eq_attr "has_likely" "no")))
+  [(eq_attr "can_delay" "yes")
+   (nil)
+   (nil)])
+
 (define_delay (eq_attr "type" "jump")
   [(eq_attr "can_delay" "yes")
    (nil)
@@ -5556,6 +5579,50 @@
 	(if_then_else (match_operand 0)
 		      (label_ref (match_operand 1))
 		      (pc)))])
+
+;; Branch if bit is set/clear.
+
+(define_insn "*branch_bit<bbv><mode>"
+  [(set (pc)
+	(if_then_else
+	 (equality_op (zero_extract:GPR
+		       (match_operand:GPR 1 "register_operand" "d")
+		       (const_int 1)
+		       (match_operand 2 "const_int_operand" ""))
+		      (const_int 0))
+	 (label_ref (match_operand 0 ""))
+	 (pc)))]
+  "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)"
+{
+  return
+    mips_output_conditional_branch (insn, operands,
+				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"),
+				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"));
+}
+  [(set_attr "type"	  "branch")
+   (set_attr "mode"	  "none")
+   (set_attr "has_likely" "no")])
+
+(define_insn "*branch_bit<bbv><mode>_inverted"
+  [(set (pc)
+	(if_then_else
+	 (equality_op (zero_extract:GPR
+		       (match_operand:GPR 1 "register_operand" "d")
+		       (const_int 1)
+		       (match_operand 2 "const_int_operand" ""))
+		      (const_int 0))
+	 (pc)
+	 (label_ref (match_operand 0 ""))))]
+  "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)"
+{
+  return
+    mips_output_conditional_branch (insn, operands,
+				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"),
+				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"));
+}
+  [(set_attr "type"	  "branch")
+   (set_attr "mode"	  "none")
+   (set_attr "has_likely" "no")])
 \f
 ;;
 ;;  ....................
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-1.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-1.c	2008-08-26 17:29:49.000000000 -0700
@@ -0,0 +1,55 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-final { scan-assembler-times "\tbbit1\t" 4 } } */
+/* { dg-final { scan-assembler-times "\tbbit0\t" 2 } } */
+/* { dg-final { scan-assembler-not "andi\t" } } */
+
+NOMIPS16 void
+f1 (long long i)
+{
+  if (i & 0x80)
+    foo ();
+}
+
+NOMIPS16 void
+f2 (int i)
+{
+  if (!(i & 0x80))
+    foo ();
+}
+
+NOMIPS16 void
+f3 (int i)
+{
+  if (i % 2)
+    foo ();
+}
+
+NOMIPS16 void
+f4 (int i)
+{
+  if (i & 1)
+    foo ();
+}
+
+NOMIPS16 void
+f5 (long long i)
+{
+  if ((i >> 3) & 1)
+    foo ();
+}
+
+unsigned long long r;
+
+NOMIPS16 static inline __attribute__((always_inline)) int
+test_bit(unsigned long long nr, const unsigned long long *addr)
+{
+  return 1UL & (addr[nr >> 6] >> (nr & 63ULL));
+}
+
+NOMIPS16 void
+f6 ()
+{
+  if (!test_bit(0, &r))
+    g ();
+}
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-2.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-2.c	2008-08-26 17:29:49.000000000 -0700
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon -mbranch-likely" } */
+/* { dg-final { scan-assembler "\tbbit\[01\]\t" } } */
+/* { dg-final { scan-assembler-not "\tbbit\[01\]l\t" } } */
+/* { dg-final { scan-assembler "\tbnel\t" } } */
+/* { dg-final { scan-assembler-not "\tbne\t" } } */
+
+NOMIPS16 int
+f (int n, int i)
+{
+  int s = 0;
+  for (; i & 1; i++)
+    s += i;
+  return s;
+}
+
+NOMIPS16 int
+g (int n, int i)
+{
+  int s = 0;
+  for (i = 0; i < n; i++)
+    s += i;
+  return s;
+}
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-3.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-bbit-3.c	2008-08-26 17:29:49.000000000 -0700
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
+/* { dg-final { scan-assembler-not "ext\t" } } */
+
+void abort (void);
+void exit (int);
+
+typedef unsigned long long ulong64;
+
+typedef struct bitfield_s {
+  ulong64 a:1;
+  ulong64 b:29;
+  ulong64 c:1;
+  ulong64 d:15;
+  ulong64 f:18;
+} bitfield_t;
+
+bitfield_t bar;
+
+NOMIPS16 void
+f ()
+{
+  foo(&bar);
+  if (bar.a != 0x1)
+    abort ();
+  else if (!bar.c)
+    abort ();
+  else
+    exit (0);
+}

Re: [PATCH, MIPS] Add bbit* Octeon instructions

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford wrote:
>> Adam Nemet <anemet@caviumnetworks.com> writes:
>>> If that's acceptable I'd like to sort out whether we need the SI-mode
>>> extraction while fixing this miscompilation.  And then depending on the
>>> outcome also remove SI mode from the bbit pattern.
>> 
>> Won't you still need SImode for 32-bit targets?  (I realise you're
>> planning on removing the o32 multilib, but still... it seems odd
>> remove SImode stuff from 64-bit targets.)
>
> What I meant was to remove the SI pattern for TARGET_64BIT not for
> !TARGET_64BIT (o32).  Basically writing these patterns with a new mode
> iterator:
>
>   (define_mode_iterator WORD [(SI "!TARGET_64BIT") (DI "TARGET_64BIT)])
>
> (In fact I had a parallel version of this patch written and tested with
> WORD and the bbit tests were passing with o32 and I had the same number
> of bbit instructions in libgcj.so.)

OK, but it still seems odd to remove SImode stuff from 64-bit targets.
They generally ought to allow both.  The concern isn't so much the number
of bbit instructions, but the number of mode changes (and the effect they
can have on optimisation).

The target can logically do both 32-bit and 64-bit ops, so I think the
fallback position is to keep things simple and stick to :GPR.

>> Can't you simply use:
>> 
>>   (set_attr "branch_likely" "no")
>> 
>> avoiding the extra define_delay and "has_likely" attribute?
>
> No and I should have really explained that in the email because this had
> been my first reaction as well.  The annul predicates test the
> *candidate instruction* for the delay slot and *not* the branch
> instruction.  Only the actual define_delay predicate is a test on the
> branch instruction so the check needs to go there.

Ah, thanks for the explanation.  But "branch_likely" isn't conceptually
a property of the delay slot insn, so I still think it makes sense to
reuse it.  I.e. rather than:

>  (define_delay (and (eq_attr "type" "branch")
> -		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "yes")))
>    [(eq_attr "can_delay" "yes")
>     (nil)
>     (and (eq_attr "branch_likely" "yes")
>  	(eq_attr "can_delay" "yes"))])
>  
> +;; Branches that don't have likely variants do not annul on false.
> +(define_delay (and (eq_attr "type" "branch")
> +		   (and (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> +			(eq_attr "has_likely" "no")))
> +  [(eq_attr "can_delay" "yes")
> +   (nil)
> +   (nil)])
> +
>  (define_delay (eq_attr "type" "jump")
>    [(eq_attr "can_delay" "yes")
>     (nil)

do:

(define_delay (and (eq_attr "type" "branch")
		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
		   (eq_attr "branch_likely" "yes"))
  [(eq_attr "can_delay" "yes")
   (nil)
   (eq_attr "can_delay" "yes")])

;; Branches that don't have likely variants do not annul on false.
(define_delay (and (eq_attr "type" "branch")
		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
		   (eq_attr "branch_likely" "no"))
  [(eq_attr "can_delay" "yes")
   (nil)
   (nil)])

(I believe "and" can take more than 2 arguments in this context.)

Untested. ;)

OK with that change, if it works.

Richard

Re: [PATCH, MIPS] Add bbit* Octeon instructions

[Adam Nemet]

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

Richard Sandiford wrote:
> OK, but it still seems odd to remove SImode stuff from 64-bit targets.
> They generally ought to allow both.  The concern isn't so much the number
> of bbit instructions, but the number of mode changes (and the effect they
> can have on optimisation).

OK.  I think the argument that even if the backend does not emit SI mode
zero_extract the middle end is free to do so is a convincing one.
Trying to be a minimalist, I was adding tests to try to catch this.

> The target can logically do both 32-bit and 64-bit ops, so I think the
> fallback position is to keep things simple and stick to :GPR.

Sure.  To describe the HW as having bbit SI is certainly more accurate.

> (define_delay (and (eq_attr "type" "branch")
> 		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> 		   (eq_attr "branch_likely" "yes"))
>   [(eq_attr "can_delay" "yes")
>    (nil)
>    (eq_attr "can_delay" "yes")])
> 
> ;; Branches that don't have likely variants do not annul on false.
> (define_delay (and (eq_attr "type" "branch")
> 		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
> 		   (eq_attr "branch_likely" "no"))
>   [(eq_attr "can_delay" "yes")
>    (nil)
>    (nil)])
> 
> (I believe "and" can take more than 2 arguments in this context.)

Sure, it works.  Also for some reason I thought "and" was not completely
like in Lisp.

> OK with that change, if it works.

I had to remove const from the definition of branch_likely.  This is
what I committed after reboostrapping and retesting.

Adam


[-- Attachment #2: bbit-3.patch --]
[-- Type: text/x-patch, Size: 8568 bytes --]

	* config/mips/mips.h (ISA_HAS_BBIT): New macro.
	* config/mips/mips.md (branch_likely): Remove const.  Fix
	comment formatting.
	(define_delay for type "branch"): Change to only apply for branch
	with likely variant.
	(define_delay for type "branch" and "branch_likely" no).  New delay
	definition.
	(equality_op): New code iterator.
	(bbv, bbinv): New code attributes.
	(*branch_bit<bbv><mode>, *branch_bit<bbv><mode>_inverted): New
	patterns.

testsuite/
	* gcc.target/mips/octeon-bbit-1.c: New test.
	* gcc.target/mips/octeon-bbit-2.c: New test.
	* gcc.target/mips/octeon-bbit-3.c: New test.

Index: gcc/config/mips/mips.h
===================================================================
*** gcc.orig/config/mips/mips.h	2008-08-28 16:46:50.000000000 -0700
--- gcc/config/mips/mips.h	2008-08-28 16:49:41.000000000 -0700
*************** enum mips_code_readable_setting {
*** 1006,1011 ****
--- 1006,1014 ----
     ? TARGET_LLSC && !TARGET_MIPS16	\
     : ISA_HAS_LL_SC)
  
+ /* ISA includes the bbit* instructions.  */
+ #define ISA_HAS_BBIT		TARGET_OCTEON
+ 
  /* ISA includes the pop instruction.  */
  #define ISA_HAS_POP		TARGET_OCTEON
  \f
Index: gcc/config/mips/mips.md
===================================================================
*** gcc.orig/config/mips/mips.md	2008-08-28 16:49:17.000000000 -0700
--- gcc/config/mips/mips.md	2008-08-28 16:53:59.000000000 -0700
***************
*** 599,610 ****
  		(const_string "yes")
  		(const_string "no")))
  
! ;; Attribute defining whether or not we can use the branch-likely instructions
  (define_attr "branch_likely" "no,yes"
!   (const
!    (if_then_else (ne (symbol_ref "GENERATE_BRANCHLIKELY") (const_int 0))
! 		 (const_string "yes")
! 		 (const_string "no"))))
  
  ;; True if an instruction might assign to hi or lo when reloaded.
  ;; This is used by the TUNE_MACC_CHAINS code.
--- 599,610 ----
  		(const_string "yes")
  		(const_string "no")))
  
! ;; Attribute defining whether or not we can use the branch-likely
! ;; instructions.
  (define_attr "branch_likely" "no,yes"
!   (if_then_else (ne (symbol_ref "GENERATE_BRANCHLIKELY") (const_int 0))
! 		(const_string "yes")
! 		(const_string "no")))
  
  ;; True if an instruction might assign to hi or lo when reloaded.
  ;; This is used by the TUNE_MACC_CHAINS code.
***************
*** 788,793 ****
--- 788,796 ----
  ;; by swapping the operands.
  (define_code_iterator swapped_fcond [ge gt unge ungt])
  
+ ;; Equality operators.
+ (define_code_iterator equality_op [eq ne])
+ 
  ;; These code iterators allow the signed and unsigned scc operations to use
  ;; the same template.
  (define_code_iterator any_gt [gt gtu])
***************
*** 848,853 ****
--- 851,862 ----
  				 (unge "ule")
  				 (ungt "ult")])
  
+ ;; The value of the bit when the branch is taken for branch_bit patterns.
+ ;; Comparison is always against zero so this depends on the operator.
+ (define_code_attr bbv [(eq "0") (ne "1")])
+ 
+ ;; This is the inverse value of bbv.
+ (define_code_attr bbinv [(eq "1") (ne "0")])
  \f
  ;; .........................
  ;;
***************
*** 856,866 ****
  ;; .........................
  
  (define_delay (and (eq_attr "type" "branch")
! 		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0)))
    [(eq_attr "can_delay" "yes")
     (nil)
!    (and (eq_attr "branch_likely" "yes")
! 	(eq_attr "can_delay" "yes"))])
  
  (define_delay (eq_attr "type" "jump")
    [(eq_attr "can_delay" "yes")
--- 865,883 ----
  ;; .........................
  
  (define_delay (and (eq_attr "type" "branch")
! 		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
! 		   (eq_attr "branch_likely" "yes"))
    [(eq_attr "can_delay" "yes")
     (nil)
!    (eq_attr "can_delay" "yes")])
! 
! ;; Branches that don't have likely variants do not annul on false.
! (define_delay (and (eq_attr "type" "branch")
! 		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))
! 		   (eq_attr "branch_likely" "no"))
!   [(eq_attr "can_delay" "yes")
!    (nil)
!    (nil)])
  
  (define_delay (eq_attr "type" "jump")
    [(eq_attr "can_delay" "yes")
***************
*** 5052,5057 ****
--- 5069,5118 ----
  	(if_then_else (match_operand 0)
  		      (label_ref (match_operand 1))
  		      (pc)))])
+ 
+ ;; Branch if bit is set/clear.
+ 
+ (define_insn "*branch_bit<bbv><mode>"
+   [(set (pc)
+ 	(if_then_else
+ 	 (equality_op (zero_extract:GPR
+ 		       (match_operand:GPR 1 "register_operand" "d")
+ 		       (const_int 1)
+ 		       (match_operand 2 "const_int_operand" ""))
+ 		      (const_int 0))
+ 	 (label_ref (match_operand 0 ""))
+ 	 (pc)))]
+   "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)"
+ {
+   return
+     mips_output_conditional_branch (insn, operands,
+ 				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"),
+ 				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"));
+ }
+   [(set_attr "type"	     "branch")
+    (set_attr "mode"	     "none")
+    (set_attr "branch_likely" "no")])
+ 
+ (define_insn "*branch_bit<bbv><mode>_inverted"
+   [(set (pc)
+ 	(if_then_else
+ 	 (equality_op (zero_extract:GPR
+ 		       (match_operand:GPR 1 "register_operand" "d")
+ 		       (const_int 1)
+ 		       (match_operand 2 "const_int_operand" ""))
+ 		      (const_int 0))
+ 	 (pc)
+ 	 (label_ref (match_operand 0 ""))))]
+   "ISA_HAS_BBIT && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)"
+ {
+   return
+     mips_output_conditional_branch (insn, operands,
+ 				    MIPS_BRANCH ("bbit<bbinv>", "%1,%2,%0"),
+ 				    MIPS_BRANCH ("bbit<bbv>", "%1,%2,%0"));
+ }
+   [(set_attr "type"	     "branch")
+    (set_attr "mode"	     "none")
+    (set_attr "branch_likely" "no")])
  \f
  ;;
  ;;  ....................
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-1.c
===================================================================
*** /dev/null	1970-01-01 00:00:00.000000000 +0000
--- gcc/testsuite/gcc.target/mips/octeon-bbit-1.c	2008-08-28 16:49:41.000000000 -0700
***************
*** 0 ****
--- 1,55 ----
+ /* { dg-do compile } */
+ /* { dg-mips-options "-O2 -march=octeon" } */
+ /* { dg-final { scan-assembler-times "\tbbit1\t" 4 } } */
+ /* { dg-final { scan-assembler-times "\tbbit0\t" 2 } } */
+ /* { dg-final { scan-assembler-not "andi\t" } } */
+ 
+ NOMIPS16 void
+ f1 (long long i)
+ {
+   if (i & 0x80)
+     foo ();
+ }
+ 
+ NOMIPS16 void
+ f2 (int i)
+ {
+   if (!(i & 0x80))
+     foo ();
+ }
+ 
+ NOMIPS16 void
+ f3 (int i)
+ {
+   if (i % 2)
+     foo ();
+ }
+ 
+ NOMIPS16 void
+ f4 (int i)
+ {
+   if (i & 1)
+     foo ();
+ }
+ 
+ NOMIPS16 void
+ f5 (long long i)
+ {
+   if ((i >> 3) & 1)
+     foo ();
+ }
+ 
+ unsigned long long r;
+ 
+ NOMIPS16 static inline __attribute__((always_inline)) int
+ test_bit(unsigned long long nr, const unsigned long long *addr)
+ {
+   return 1UL & (addr[nr >> 6] >> (nr & 63ULL));
+ }
+ 
+ NOMIPS16 void
+ f6 ()
+ {
+   if (!test_bit(0, &r))
+     g ();
+ }
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-2.c
===================================================================
*** /dev/null	1970-01-01 00:00:00.000000000 +0000
--- gcc/testsuite/gcc.target/mips/octeon-bbit-2.c	2008-08-28 16:49:41.000000000 -0700
***************
*** 0 ****
--- 1,24 ----
+ /* { dg-do compile } */
+ /* { dg-mips-options "-O2 -march=octeon -mbranch-likely" } */
+ /* { dg-final { scan-assembler "\tbbit\[01\]\t" } } */
+ /* { dg-final { scan-assembler-not "\tbbit\[01\]l\t" } } */
+ /* { dg-final { scan-assembler "\tbnel\t" } } */
+ /* { dg-final { scan-assembler-not "\tbne\t" } } */
+ 
+ NOMIPS16 int
+ f (int n, int i)
+ {
+   int s = 0;
+   for (; i & 1; i++)
+     s += i;
+   return s;
+ }
+ 
+ NOMIPS16 int
+ g (int n, int i)
+ {
+   int s = 0;
+   for (i = 0; i < n; i++)
+     s += i;
+   return s;
+ }
Index: gcc/testsuite/gcc.target/mips/octeon-bbit-3.c
===================================================================
*** /dev/null	1970-01-01 00:00:00.000000000 +0000
--- gcc/testsuite/gcc.target/mips/octeon-bbit-3.c	2008-08-28 16:49:41.000000000 -0700
***************
*** 0 ****
--- 1,31 ----
+ /* { dg-do compile } */
+ /* { dg-mips-options "-O2 -march=octeon" } */
+ /* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
+ /* { dg-final { scan-assembler-not "ext\t" } } */
+ 
+ void abort (void);
+ void exit (int);
+ 
+ typedef unsigned long long ulong64;
+ 
+ typedef struct bitfield_s {
+   ulong64 a:1;
+   ulong64 b:29;
+   ulong64 c:1;
+   ulong64 d:15;
+   ulong64 f:18;
+ } bitfield_t;
+ 
+ bitfield_t bar;
+ 
+ NOMIPS16 void
+ f ()
+ {
+   foo(&bar);
+   if (bar.a != 0x1)
+     abort ();
+   else if (!bar.c)
+     abort ();
+   else
+     exit (0);
+ }