diff mbox

Add TST to RX

Message ID 201007140217.o6E2Hp4w001032@greed.delorie.com
State New
Headers show

Commit Message

DJ Delorie July 14, 2010, 2:17 a.m. UTC 
This patch adds support for the TST opcode to the RX target.  The
first two patterns are the real patterns; the rest are other ways that
gcc handles "a & const", they're converted to one of the first two
forms.  Paolo: if you replace the "return 0;" in the rtlanal.c chunk
with an abort(), you can see where the zero_extend comes from.  GCC
uses that for single-bit tests other than bit 0.

Ok to apply?

	* rtlanal.c (canonicalize_condition): Exit early if we're passed
	something other than a condition.

	* config/rx/predicates.md (rx_constshift_operand): New.
	(rx_onebit_operand): New.
	* config/rx/rx.md (zs_cond): New.
	(*tstbranchsi4_<code>): New.
	(*tstbranchsi4_<code>): New.
	(*tstbranchsi4b_eq): New.
	(*tstbranchsi4m_eq): New.
	(*tstbranchsi4m_ne): New.
	(*tstbranchsi4a_ne): New.
	(*tstbranchsi4a_eq): New.

Comments

Paolo Bonzini July 14, 2010, 8:21 a.m. UTC | #1 
On 07/14/2010 04:17 AM, DJ Delorie wrote:
> +  [(set (pc)
> +	(if_then_else (zero_extract:SI (xor:SI (match_operand:SI  0 "register_operand"  "r")
> +					       (match_operand 1 "immediate_operand" "i"))
> +				       (const_int 1)
> +				       (match_operand  2 "rx_constshift_operand" "i"))
> +		      (label_ref (match_operand        3 "" ""))
> +		      (pc)))
> +   ]

This seems wrong.  It should be (if_then_else (ne (zero_extract:SI 
...))).  If that one is not generated by combine (likely because ne is 
removed somewhere?) that should be fixed.

This pattern may well be the reason why canonicalize_condition is called 
with a non-comparison RTL.

Paolo
DJ Delorie July 14, 2010, 4:23 p.m. UTC | #2 
> This seems wrong.  It should be (if_then_else (ne (zero_extract:SI 
> ...))).  If that one is not generated by combine (likely because ne is 
> removed somewhere?) that should be fixed.
> 
> This pattern may well be the reason why canonicalize_condition is called 
> with a non-comparison RTL.

It is caused by combine.  I put a debug_rtx() after
recog_for_combine() to see what gcc is expecting, and that's the
pattern it's looking for.

Feel free to figure *that* one out too :-)
DJ Delorie July 14, 2010, 4:59 p.m. UTC | #3 
Here's a test case that covered all the patterns I used...

extern void f1();
extern void f2();
extern void f3();
extern void f4();

void
func (int a)
{
  if (a & 4)
    f1();
  else if (a & 1)
    f2();
  else if (a & 2)
    f3();
  else
    f4();
}

#define T(n,x) f_##n (int a) { if (x) foo(); bar(); } fn_##n (int a) { if (!(x)) foo(); bar(); }
T(1, a & 1)
T(2, a & 2)
T(3, a & 3)
T(4, a & 4)
T(5, a & 5)
T(6, a & 8)
T(s, a & 0x80000000)
kazuhiro inaoka July 15, 2010, 9:52 a.m. UTC | #4 
DJ Delorie wrote:
> This patch adds support for the TST opcode to the RX target.  The
> first two patterns are the real patterns; the rest are other ways that
> gcc handles "a & const", they're converted to one of the first two
> forms.  Paolo: if you replace the "return 0;" in the rtlanal.c chunk
> with an abort(), you can see where the zero_extend comes from.  GCC
> uses that for single-bit tests other than bit 0.
>
> Ok to apply?
>
> 	* rtlanal.c (canonicalize_condition): Exit early if we're passed
> 	something other than a condition.
>
> 	* config/rx/predicates.md (rx_constshift_operand): New.
> 	(rx_onebit_operand): New.
> 	* config/rx/rx.md (zs_cond): New.
> 	(*tstbranchsi4_<code>): New.
> 	(*tstbranchsi4_<code>): New.
> 	(*tstbranchsi4b_eq): New.
> 	(*tstbranchsi4m_eq): New.
> 	(*tstbranchsi4m_ne): New.
> 	(*tstbranchsi4a_ne): New.
> 	(*tstbranchsi4a_eq): New.
This patch causes the following testsuite failure.

FAIL: gcc.c-torture/compile/pr32482.c -O1 (internal compiler error)

FAIL: gcc.c-torture/unsorted/shft.c, -O1 (internal compiler error)
FAIL: gcc.c-torture/unsorted/shft.c, -O2 (internal compiler error)
FAIL: gcc.c-torture/unsorted/shft.c, -O3 -fomit-frame-pointer
(internal compiler error)
FAIL: gcc.c-torture/unsorted/shft.c, -O3 -g (internal compiler error)
FAIL: gcc.c-torture/unsorted/shft.c, -Os (internal compiler error)

rx-elf-gcc -S -O1 pr32482.c
pr32482.c: In function 'drain_cpu_caches':
pr32482.c:23:1: internal compiler error: in reverse_condition, at jump.c:477
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

rx-elf-gcc -S -O3 shft.c
shft.c: In function 'foo':
shft.c:9:1: internal compiler error: in swap_condition, at jump.c:566
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

Regards,
Kazuhiro Inaoka
Paolo Bonzini July 15, 2010, 11:41 a.m. UTC | #5 
On 07/15/2010 11:52 AM, kazuhiro inaoka wrote:
>
> rx-elf-gcc -S -O1 pr32482.c
> pr32482.c: In function 'drain_cpu_caches':
> pr32482.c:23:1: internal compiler error: in reverse_condition, at
> jump.c:477
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
>
> rx-elf-gcc -S -O3 shft.c
> shft.c: In function 'foo':
> shft.c:9:1: internal compiler error: in swap_condition, at jump.c:566
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.

If these are (as it is likely) called with a ZERO_EXTRACT argument, this 
shows again that the pattern are wrong and that the real bug lies 
wherever (if_then_else (zero_extract)) is generated by combine.

Paolo
Paolo Bonzini July 16, 2010, 7:24 p.m. UTC | #6 
On 07/14/2010 06:23 PM, DJ Delorie wrote:
>> This seems wrong.  It should be (if_then_else (ne (zero_extract:SI
>> ...))).  If that one is not generated by combine (likely because ne is
>> removed somewhere?) that should be fixed.
>>
>> This pattern may well be the reason why canonicalize_condition is called
>> with a non-comparison RTL.
>
> It is caused by combine.  I put a debug_rtx() after
> recog_for_combine() to see what gcc is expecting, and that's the
> pattern it's looking for.
>
> Feel free to figure *that* one out too :-)

It's the same thing.  Here:

(define_expand "cbranchsi4"
   [(set (cc0) (compare:CC (match_operand:SI 1 "register_operand")
                           (match_operand:SI 2 "rx_source_operand")))
    (set (pc)
         (if_then_else (match_operator:SI  0 "comparison_operator"
                                           [(cc0) (const_int 0)])
                       (label_ref (match_operand 3 ""))
                       (pc)))]


the match_operator should be modeless.  (ne:SI (zero_extract:SI) 
(const_int 0)) is indeed the same as just the zero_extract, but not with 
a modeless ne.  So combine is correct.

Paolo
diff mbox

Patch

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 162148)
+++ rtlanal.c	(working copy)
@@ -4695,12 +4695,17 @@  canonicalize_condition (rtx insn, rtx co
   rtx tem;
   rtx op0, op1;
   int reverse_code = 0;
   enum machine_mode mode;
   basic_block bb = BLOCK_FOR_INSN (insn);
 
+  /* Single-bit tests sometimes use logic ops to generate the
+     condition, rather than comparisons.  */
+  if (! COMPARISON_P (cond))
+    return 0;
+
   code = GET_CODE (cond);
   mode = GET_MODE (cond);
   op0 = XEXP (cond, 0);
   op1 = XEXP (cond, 1);
 
   if (reverse)
Index: config/rx/predicates.md
===================================================================
--- config/rx/predicates.md	(revision 162148)
+++ config/rx/predicates.md	(working copy)
@@ -42,12 +42,29 @@ 
     if (CONST_INT_P (op))
       return IN_RANGE (INTVAL (op), 0, 31);
     return true;
   }
 )
 
+(define_predicate "rx_constshift_operand"
+  (match_code "const_int")
+  {
+    return IN_RANGE (INTVAL (op), 0, 31);
+  }
+)
+
+;; Check that the operand is suitable for a TST inversion
+
+(define_predicate "rx_onebit_operand"
+  (match_code "const_int")
+  {
+    HOST_WIDE_INT ival = INTVAL (op);
+    return (ival && (ival & (ival-1)) == 0);
+  }
+)
+
 ;; Check that the operand is suitable as the source operand
 ;; for a logic or arithmeitc instruction.  Registers, integers
 ;; and a restricted subset of memory addresses are allowed.
 
 (define_predicate "rx_source_operand"
   (match_code "const_int,const_double,const,symbol_ref,label_ref,reg,mem")
Index: config/rx/rx.md
===================================================================
--- config/rx/rx.md	(revision 162148)
+++ config/rx/rx.md	(working copy)
@@ -21,12 +21,15 @@ 
 
 ;; This code iterator allows all branch instructions to
 ;; be generated from a single define_expand template.
 (define_code_iterator most_cond [eq ne gt ge lt le gtu geu ltu leu
 				 unordered ordered ])
 
+;; Likewise, but only the ones that use Z or S.
+(define_code_iterator zs_cond [eq ne gtu geu ltu leu ])
+
 ;; This code iterator is used for sign- and zero- extensions.
 (define_mode_iterator small_int_modes [(HI "") (QI "")])
 
 ;; We do not handle DFmode here because it is either
 ;; the same as SFmode, or if -m64bit-doubles is active
 ;; then all operations on doubles have to be handled by
@@ -186,12 +189,190 @@ 
   emit_jump_insn (gen_conditional_branch (operands[2],
   		 gen_rtx_fmt_ee (<most_cond:CODE>, CCmode,
 				 gen_rtx_REG (CCmode, CC_REG), const0_rtx)));
   "
 )
 
+;; -----------------------------------------------------------------------------
+;; These two are the canonical TST/branch insns.  However, GCC
+;; generates a wide variety of tst-like patterns, we catch those
+;; below.
+(define_insn_and_split "*tstbranchsi4_<code>"
+  [(set (pc)
+	(if_then_else (zs_cond:SI (and:SI (match_operand:SI  0 "register_operand"  "r")
+					  (match_operand:SI  1 "rx_source_operand" "riQ"))
+				  (const_int 0))
+		      (label_ref (match_operand 2 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  "reload_completed"
+  [(const_int 0)]
+  "
+  emit_insn (gen_tstsi (operands[0], operands[1]));
+  
+  emit_jump_insn (gen_conditional_branch (operands[2],
+  		 gen_rtx_fmt_ee (<zs_cond:CODE>, CCmode,
+				 gen_rtx_REG (CCmode, CC_REG), const0_rtx)));
+  "
+)
+
+;; Inverse of above
+(define_insn_and_split "*tstbranchsi4_<code>"
+  [(set (pc)
+	(if_then_else (zs_cond:SI (and:SI (match_operand:SI  0 "register_operand"  "r")
+					  (match_operand:SI  1 "rx_source_operand" "riQ"))
+				  (const_int 0))
+		      (pc)
+		      (label_ref (match_operand 2 "" ""))))
+   ]
+  ""
+  "#"
+  "reload_completed"
+  [(const_int 0)]
+  "
+  emit_insn (gen_tstsi (operands[0], operands[1]));
+  
+  emit_jump_insn (gen_conditional_branch (operands[2],
+  		 gen_rtx_fmt_ee (reverse_condition (<zs_cond:CODE>), CCmode,
+				 gen_rtx_REG (CCmode, CC_REG), const0_rtx)));
+  "
+)
+
+;; Various other ways that GCC codes "var & const"
+
+(define_insn_and_split "*tstbranchsi4b_eq"
+  [(set (pc)
+	(if_then_else (zero_extract:SI (xor:SI (match_operand:SI  0 "register_operand"  "r")
+					       (match_operand 1 "immediate_operand" "i"))
+				       (const_int 1)
+				       (match_operand  2 "rx_constshift_operand" "i"))
+		      (label_ref (match_operand        3 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (eq:SI (and:SI (match_dup  0)
+				     (match_dup 4))
+			     (const_int 0))
+		      (label_ref (match_dup 3))
+		      (pc)))
+   ]
+  "operands[4] = GEN_INT (1 << INTVAL (operands[2]));"
+)
+
+(define_insn_and_split "*tstbranchsi4b_ne"
+  [(set (pc)
+	(if_then_else (zero_extract:SI (match_operand:SI  0 "register_operand"  "r")
+				       (match_operand  1 "rx_constshift_operand" "i")
+				       (match_operand  2 "rx_constshift_operand" "i"))
+		      (label_ref (match_operand        3 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (ne:SI (and:SI (match_dup  0)
+				     (match_dup 4))
+			     (const_int 0))
+		      (label_ref (match_dup 3))
+		      (pc)))
+   ]
+  "operands[4] = GEN_INT (((1 << INTVAL (operands[1]))-1) << INTVAL (operands[2]));"
+)
+
+(define_insn_and_split "*tstbranchsi4m_eq"
+  [(set (pc)
+	(if_then_else (eq:SI (zero_extract:SI (match_operand:SI  0 "register_operand"  "r")
+					      (match_operand  1 "rx_constshift_operand" "i")
+					      (match_operand  2 "rx_constshift_operand" "i"))
+			     (const_int 0))
+		      (label_ref (match_operand        3 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (eq:SI (and:SI (match_dup  0)
+				     (match_dup 4))
+			     (const_int 0))
+		      (label_ref (match_dup 3))
+		      (pc)))
+   ]
+  "operands[4] = GEN_INT (((1 << INTVAL (operands[1]))-1) << INTVAL (operands[2]));"
+)
+
+(define_insn_and_split "*tstbranchsi4m_ne"
+  [(set (pc)
+	(if_then_else (ne:SI (zero_extract:SI (match_operand:SI  0 "register_operand"  "r")
+					      (match_operand  1 "rx_constshift_operand" "i")
+					      (match_operand  2 "rx_constshift_operand" "i"))
+			     (const_int 0))
+		      (label_ref (match_operand        3 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (ne:SI (and:SI (match_dup  0)
+				     (match_dup 4))
+			     (const_int 0))
+		      (label_ref (match_dup 3))
+		      (pc)))
+   ]
+  "operands[4] = GEN_INT (((1 << INTVAL (operands[1]))-1) << INTVAL (operands[2]));"
+)
+
+(define_insn_and_split "*tstbranchsi4a_ne"
+  [(set (pc)
+	(if_then_else (and:SI (match_operand:SI  0 "register_operand"  "r")
+			      (match_operand 1 "immediate_operand" "i"))
+		      (label_ref (match_operand 2 "" ""))
+		      (pc)))
+   ]
+  ""
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (ne:SI (and:SI (match_dup  0)
+				     (match_dup 1))
+			     (const_int 0))
+		      (label_ref (match_dup 2))
+		      (pc)))
+   ]
+  ""
+)
+
+(define_insn_and_split "*tstbranchsi4a_eq"
+  [(set (pc)
+	(if_then_else (and:SI (not:SI (match_operand:SI  0 "register_operand"  "r"))
+			      (match_operand 1 "immediate_operand" "i"))
+		      (label_ref (match_operand 2 "" ""))
+		      (pc)))
+   ]
+  "rx_onebit_operand (operands[1], VOIDmode)"
+  "#"
+  ""
+  [(set (pc)
+	(if_then_else (eq:SI (and:SI (match_dup  0)
+				     (match_dup 1))
+			     (const_int 0))
+		      (label_ref (match_dup 2))
+		      (pc)))
+   ]
+  ""
+)
+
+;; -----------------------------------------------------------------------------
+
 (define_expand "cbranchsf4"
   [(set (pc)
 	(if_then_else (match_operator:SF 0 "comparison_operator"
 					 [(match_operand:SF 1 "register_operand")
 					  (match_operand:SF 2 "rx_source_operand")])
 		      (label_ref (match_operand 3 ""))