MIPS bug fix for Broadcom SB-1A

James E Wilson wilson@specifix.com
Mon Jun 26 21:14:00 GMT 2006


Trying to generate mips-linux O32 SB-1A code fails with an ICE in
store_data_bypass_p.  The problem is that the linux O32 support is still
using a cprestore pattern that uses an UNSPEC_VOLATILE, and which has an
instruction type of store.  This causes it to be passed to the
store_data_bypass_p function, which then calls abort because it can't
find the store address.  The prev insn that caused this is the
instruction that adjusts the stack pointer downwards before the
cprestore.

This failure does not happen for the SB-1 port, because the
ir_simple_sb1_alu pattern does not appear in a bypass that uses
store_data_bypass_p.  The ir_simple_sb1a_alu pattern however does.

There are a number of possible solutions here.  I chose a simple one.  I
added a mips_store_data_bypass_p function that handles cprestore, and
then passes all other insns off to the standard store_data_bypass_p
function.  While working on this, I noticed that I accidentally got the
sense of the tests wrong.  I had the sense right in my original patch
that added my own store address check function, but when I modified it
to use the standard store_data_bypass_p I missed the fact that it was
doing something slightly different.  So I fixed this also in this patch.

There are also other possible solutions.  The cprestore pattern could be
modified to stop using an UNSPEC_VOLATILE.  I'm not sure what the effect
of doing that would be though.  The mips_store_data_bypass_p function is
safer.  We could fix the standard store_data_bypass_p function to handle
cprestore, but it seems wrong to let mips backend cruft leak into the
recog.c file.

Just now while writing this up, I noticed that the 24k.md DFA scheduler
has yet another solution.  It adds a special pattern to match the
cprestore pattern, so it won't end up matching the bypass pattern.  This
seems somewhat reasonable, except it means all affected DFA schedulers
have to have a special cprestore pattern.  My approach means that we can
modify them all to use mips_store_data_bypass_p, and no special DFA
sched patterns are needed.

Comments?  I am willing to adopt whatever solution people think is best.

As with the previous patch, this was tested with a mips-linux O32
--with-{arch,tune}=sb1a C and C++ bootstrap and make check.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com
-------------- next part --------------
2006-06-24  James E Wilson  <wilson@specifix.com>

	* config/mips/mips-protos.h (mips_store_data_bypass_p): New.
	* config/mips/mips.c (mips_store_data_bypass_p): New.
	* config/mips/sb1.md: Use mips_store_data_bypass_p instead of
	store_data_bypass_p.

Index: mips-protos.h
===================================================================
--- mips-protos.h	(revision 114960)
+++ mips-protos.h	(working copy)
@@ -243,6 +243,7 @@ extern const char *mips_output_order_con
 extern const char *mips_output_division (const char *, rtx *);
 extern unsigned int mips_hard_regno_nregs (int, enum machine_mode);
 extern bool mips_linked_madd_p (rtx, rtx);
+extern int mips_store_data_bypass_p (rtx, rtx);
 extern rtx mips_prefetch_cookie (rtx, rtx);
 
 extern void irix_asm_output_align (FILE *, unsigned);
--- mips.c.orig	2006-06-24 20:40:03.302531482 -0700
+++ mips.c	2006-06-24 20:45:15.236987818 -0700
@@ -9944,6 +9944,20 @@
 
   return 0;
 }
+
+/* Implements a store data bypass check.  We need this because the cprestore
+   pattern is type store, but defined using an UNSPEC.  This UNSPEC causes the
+   default routine to abort.  We just return false for that case.  */
+/* ??? Should try to give a better result here than assuming false.  */
+
+int
+mips_store_data_bypass_p (rtx out_insn, rtx in_insn)
+{
+  if (GET_CODE (PATTERN (in_insn)) == UNSPEC_VOLATILE)
+    return false;
+
+  return ! store_data_bypass_p (out_insn, in_insn);
+}
 
 /* Given that we have an rtx of the form (prefetch ... WRITE LOCALITY),
    return the first operand of the associated "pref" or "prefx" insn.  */
--- sb1.md.orig	2006-06-24 20:40:16.612288740 -0700
+++ sb1.md	2006-06-24 20:45:15.238987481 -0700
@@ -203,7 +203,7 @@
   "ir_sb1_load,ir_sb1a_load,ir_sb1_fpload,ir_sb1_fpload_32bitfp,
    ir_sb1_fpidxload,ir_sb1_fpidxload_32bitfp"
   "ir_sb1_store,ir_sb1_fpstore,ir_sb1_fpidxstore"
-  "store_data_bypass_p")
+  "mips_store_data_bypass_p")
 
 ;; On SB-1, simple alu instructions can execute on the LS1 unit.
 
@@ -276,7 +276,7 @@
 (define_bypass 5
   "ir_sb1a_simple_alu,ir_sb1_alu,ir_sb1_alu_0,ir_sb1_mfhi,ir_sb1_mflo"
   "ir_sb1_store,ir_sb1_fpstore,ir_sb1_fpidxstore"
-  "store_data_bypass_p")
+  "mips_store_data_bypass_p")
 
 ;; mf{hi,lo} is 1 cycle.  
 
@@ -340,7 +340,7 @@
 (define_bypass 7
   "ir_sb1_mulsi,ir_sb1_muldi"
   "ir_sb1_store,ir_sb1_fpstore,ir_sb1_fpidxstore"
-  "store_data_bypass_p")
+  "mips_store_data_bypass_p")
 
 ;; The divide unit is not pipelined.  Divide busy is asserted in the 4th
 ;; cycle, and then deasserted on the latency cycle.  So only one divide at


More information about the Gcc-patches mailing list