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

[Adam Nemet <anemet at caviumnetworks dot com>]

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

	* 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
 
 /* 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")])
 
 ;; .........................
 ;;
@@ -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")])
 
 ;;
 ;;  ....................
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);
+}