diff mbox

[MIPS] fix MIPS16 jump table overflow

Message ID 50359228.3070308@codesourcery.com
State New
Headers show

Commit Message

Sandra Loosemore Aug. 23, 2012, 2:15 a.m. UTC 
On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>
> Would be nice to add a compile test for -mabi=64 just to make sure
> that Pmode == DImode works.  A copy of an existing test like
> code-readable-1.c would be fine.

I'm having problems with this part -- it seems like every combination of 
options with -mabi=64 I've tried with code-readable-1.c complains about 
something-or-another being incompatible.  The closest I've come is 
"-mabi=64 -march=mips64 -msoft-float", which is accepted by the 
mipsisa32r2-sde-elf build I'm using for testing, but when I add -O to 
that I get an unrelated ICE:

code-readable-1.c: In function 'bar':
code-readable-1.c:25:1: error: unrecognizable insn:
(insn 17 5 18 2 (set (reg/i:DI 2 $2)
         (high:DI (const:DI (unspec:DI [
                         (symbol_ref:DI ("k") [flags 0x40] <var_decl 
0xf72d14ac k>)
                     ] 229)))) code-readable-1.c:25 -1
      (nil))
code-readable-1.c:25:1: internal compiler error: in extract_insn, at 
recog.c:2125

And, even without -O, that combination of options gives a "sorry" on the 
mips-linux-gnu build I also have around.

Here's the revised patch that addresses the other problems you pointed 
out.  Is this part OK, at least?  It passes regression testing.

-Sandra


2012-08-22  Julian Brown  <julian@codesourcery.com>
	    Sandra Loosemore  <sandra@codesourcery.com>

	gcc/
	* config/mips/mips.md
	(UNSPEC_CASESI_DISPATCH): New.
	(MIPS16_T_REGNUM): New constant.
	(tablejump): Don't use for MIPS16_SHORT_JUMP_TABLES.
	(casesi): New.
	(casesi_internal_mips16_<mode>): New.
	* config/mips/mips.c (mips16_split_long_branches): Adjust test
	to ignore casesi jump tables.
	* config/mips/mips.h (TARGET_MIPS16_SHORT_JUMP_TABLES): Update
	comment.
	(CASE_VECTOR_MODE): Use SImode unconditionally.
	(CASE_VECTOR_SHORTEN_MODE): Define.
	(ASM_OUTPUT_ADDR_DIFF_ELT): Output word-sized addr_diff_elts
	when necessary for MIPS16_SHORT_JUMP_TABLES.

	gcc/testsuite/
	* gcc.target/mips/code-readable-1.c: Add -O to options.

Comments

Richard Sandiford Aug. 23, 2012, 6:42 a.m. UTC | #1 
Sandra Loosemore <sandra@codesourcery.com> writes:
> 2012-08-22  Julian Brown  <julian@codesourcery.com>
> 	    Sandra Loosemore  <sandra@codesourcery.com>
>
> 	gcc/
> 	* config/mips/mips.md
> 	(UNSPEC_CASESI_DISPATCH): New.
> 	(MIPS16_T_REGNUM): New constant.
> 	(tablejump): Don't use for MIPS16_SHORT_JUMP_TABLES.
> 	(casesi): New.
> 	(casesi_internal_mips16_<mode>): New.
> 	* config/mips/mips.c (mips16_split_long_branches): Adjust test
> 	to ignore casesi jump tables.
> 	* config/mips/mips.h (TARGET_MIPS16_SHORT_JUMP_TABLES): Update
> 	comment.
> 	(CASE_VECTOR_MODE): Use SImode unconditionally.
> 	(CASE_VECTOR_SHORTEN_MODE): Define.
> 	(ASM_OUTPUT_ADDR_DIFF_ELT): Output word-sized addr_diff_elts
> 	when necessary for MIPS16_SHORT_JUMP_TABLES.
>
> 	gcc/testsuite/
> 	* gcc.target/mips/code-readable-1.c: Add -O to options.

OK, thanks.

Richard
Andrew Pinski Aug. 25, 2012, 5:08 a.m. UTC | #2 
On Wed, Aug 22, 2012 at 7:15 PM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>>
>>
>> Would be nice to add a compile test for -mabi=64 just to make sure
>> that Pmode == DImode works.  A copy of an existing test like
>> code-readable-1.c would be fine.
>
>
> I'm having problems with this part -- it seems like every combination of
> options with -mabi=64 I've tried with code-readable-1.c complains about
> something-or-another being incompatible.  The closest I've come is "-mabi=64
> -march=mips64 -msoft-float", which is accepted by the mipsisa32r2-sde-elf

Did you test this at all on a mips64-*-* target?  After this change
n64 jump tables are broken.
Before CASE_VECTOR_MODE was DImode for n64 .

See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54371 for why it fails.
gpdword produces a double word for n64.

For EABI64 it is ok to load 32bits because that the addresses are
32bits but for n64, it is not ok.  The load addresses are normally
above the 32bits boundary.

Thanks,
Andrew Pinski
Andrew Pinski Aug. 25, 2012, 5:14 a.m. UTC | #3 
On Fri, Aug 24, 2012 at 10:08 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Wed, Aug 22, 2012 at 7:15 PM, Sandra Loosemore
> <sandra@codesourcery.com> wrote:
>> On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>>>
>>>
>>> Would be nice to add a compile test for -mabi=64 just to make sure
>>> that Pmode == DImode works.  A copy of an existing test like
>>> code-readable-1.c would be fine.
>>
>>
>> I'm having problems with this part -- it seems like every combination of
>> options with -mabi=64 I've tried with code-readable-1.c complains about
>> something-or-another being incompatible.  The closest I've come is "-mabi=64
>> -march=mips64 -msoft-float", which is accepted by the mipsisa32r2-sde-elf
>
> Did you test this at all on a mips64-*-* target?  After this change
> n64 jump tables are broken.
> Before CASE_VECTOR_MODE was DImode for n64 .
>
> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54371 for why it fails.
> gpdword produces a double word for n64.
>
> For EABI64 it is ok to load 32bits because that the addresses are
> 32bits but for n64, it is not ok.  The load addresses are normally
> above the 32bits boundary.

I am testing a patch which changes CASE_VECTOR_MODE to be:
#define CASE_VECTOR_MODE ((mips_abi == ABI_64) ? DImode : SImode)

Thanks,
Andrew Pinski
Richard Sandiford Aug. 25, 2012, 5:46 a.m. UTC | #4 
Andrew Pinski <pinskia@gmail.com> writes:
> On Fri, Aug 24, 2012 at 10:08 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Wed, Aug 22, 2012 at 7:15 PM, Sandra Loosemore
>> <sandra@codesourcery.com> wrote:
>>> On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>>>>
>>>>
>>>> Would be nice to add a compile test for -mabi=64 just to make sure
>>>> that Pmode == DImode works.  A copy of an existing test like
>>>> code-readable-1.c would be fine.
>>>
>>>
>>> I'm having problems with this part -- it seems like every combination of
>>> options with -mabi=64 I've tried with code-readable-1.c complains about
>>> something-or-another being incompatible.  The closest I've come is "-mabi=64
>>> -march=mips64 -msoft-float", which is accepted by the mipsisa32r2-sde-elf
>>
>> Did you test this at all on a mips64-*-* target?  After this change
>> n64 jump tables are broken.
>> Before CASE_VECTOR_MODE was DImode for n64 .
>>
>> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54371 for why it fails.
>> gpdword produces a double word for n64.
>>
>> For EABI64 it is ok to load 32bits because that the addresses are
>> 32bits but for n64, it is not ok.  The load addresses are normally
>> above the 32bits boundary.
>
> I am testing a patch which changes CASE_VECTOR_MODE to be:
> #define CASE_VECTOR_MODE ((mips_abi == ABI_64) ? DImode : SImode)

I think it should be:

#define CASE_VECTOR_MODE \
  (TARGET_MIPS16_SHORT_JUMP_TABLES ? SImode : ptr_mode)

#define CASE_VECTOR_SHORTEN_MODE(MIN, MAX, BODY) \
  (!TARGET_MIPS16_SHORT_JUMP_TABLES ? ptr_mode \
   : (MIN) >= -32768 && (MAX) < 32768 : HImode \
   : SImode)

The point being that the TARGET_MIPS16_SHORT_JUMP_TABLES entries
are relative, so SImode would be correct there even for n64.

I'd missed that CASE_VECTOR_MODE applied to tablejump as well
as casesi, sorry.

Richard
Andrew Pinski Aug. 25, 2012, 5:56 a.m. UTC | #5 
On Fri, Aug 24, 2012 at 10:46 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Andrew Pinski <pinskia@gmail.com> writes:
>> On Fri, Aug 24, 2012 at 10:08 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Wed, Aug 22, 2012 at 7:15 PM, Sandra Loosemore
>>> <sandra@codesourcery.com> wrote:
>>>> On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>>>>>
>>>>>
>>>>> Would be nice to add a compile test for -mabi=64 just to make sure
>>>>> that Pmode == DImode works.  A copy of an existing test like
>>>>> code-readable-1.c would be fine.
>>>>
>>>>
>>>> I'm having problems with this part -- it seems like every combination of
>>>> options with -mabi=64 I've tried with code-readable-1.c complains about
>>>> something-or-another being incompatible.  The closest I've come is "-mabi=64
>>>> -march=mips64 -msoft-float", which is accepted by the mipsisa32r2-sde-elf
>>>
>>> Did you test this at all on a mips64-*-* target?  After this change
>>> n64 jump tables are broken.
>>> Before CASE_VECTOR_MODE was DImode for n64 .
>>>
>>> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54371 for why it fails.
>>> gpdword produces a double word for n64.
>>>
>>> For EABI64 it is ok to load 32bits because that the addresses are
>>> 32bits but for n64, it is not ok.  The load addresses are normally
>>> above the 32bits boundary.
>>
>> I am testing a patch which changes CASE_VECTOR_MODE to be:
>> #define CASE_VECTOR_MODE ((mips_abi == ABI_64) ? DImode : SImode)
>
> I think it should be:
>
> #define CASE_VECTOR_MODE \
>   (TARGET_MIPS16_SHORT_JUMP_TABLES ? SImode : ptr_mode)
>
> #define CASE_VECTOR_SHORTEN_MODE(MIN, MAX, BODY) \
>   (!TARGET_MIPS16_SHORT_JUMP_TABLES ? ptr_mode \
>    : (MIN) >= -32768 && (MAX) < 32768 : HImode \
>    : SImode)
>
> The point being that the TARGET_MIPS16_SHORT_JUMP_TABLES entries
> are relative, so SImode would be correct there even for n64.
>
> I'd missed that CASE_VECTOR_MODE applied to tablejump as well
> as casesi, sorry.

I am testing the above patch right now.

Thanks,
Andrew
Sandra Loosemore Aug. 25, 2012, 3:52 p.m. UTC | #6 
On 08/24/2012 11:46 PM, Richard Sandiford wrote:
> Andrew Pinski<pinskia@gmail.com>  writes:
>> On Fri, Aug 24, 2012 at 10:08 PM, Andrew Pinski<pinskia@gmail.com>  wrote:
>>> On Wed, Aug 22, 2012 at 7:15 PM, Sandra Loosemore
>>> <sandra@codesourcery.com>  wrote:
>>>> On 08/21/2012 02:23 PM, Richard Sandiford wrote:
>>>>>
>>>>>
>>>>> Would be nice to add a compile test for -mabi=64 just to make sure
>>>>> that Pmode == DImode works.  A copy of an existing test like
>>>>> code-readable-1.c would be fine.
>>>>
>>>>
>>>> I'm having problems with this part -- it seems like every combination of
>>>> options with -mabi=64 I've tried with code-readable-1.c complains about
>>>> something-or-another being incompatible.  The closest I've come is "-mabi=64
>>>> -march=mips64 -msoft-float", which is accepted by the mipsisa32r2-sde-elf
>>>
>>> Did you test this at all on a mips64-*-* target?  After this change
>>> n64 jump tables are broken.
>>> Before CASE_VECTOR_MODE was DImode for n64 .
>>>
>>> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54371 for why it fails.
>>> gpdword produces a double word for n64.
>>>
>>> For EABI64 it is ok to load 32bits because that the addresses are
>>> 32bits but for n64, it is not ok.  The load addresses are normally
>>> above the 32bits boundary.
>>
>> I am testing a patch which changes CASE_VECTOR_MODE to be:
>> #define CASE_VECTOR_MODE ((mips_abi == ABI_64) ? DImode : SImode)
>
> I think it should be:
>
> #define CASE_VECTOR_MODE \
>    (TARGET_MIPS16_SHORT_JUMP_TABLES ? SImode : ptr_mode)
>
> #define CASE_VECTOR_SHORTEN_MODE(MIN, MAX, BODY) \
>    (!TARGET_MIPS16_SHORT_JUMP_TABLES ? ptr_mode \
>     : (MIN)>= -32768&&  (MAX)<  32768 : HImode \
>     : SImode)
>
> The point being that the TARGET_MIPS16_SHORT_JUMP_TABLES entries
> are relative, so SImode would be correct there even for n64.
>
> I'd missed that CASE_VECTOR_MODE applied to tablejump as well
> as casesi, sorry.

Yeah, sorry.  :-(  The original version of the patch I posted (where 
CASE_VECTOR_MODE was #defined to ptr_mode) had indeed been tested on 
mips64, but not the revised version that was approved for check-in.

-Sandra
diff mbox

Patch

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	(revision 190463)
+++ gcc/config/mips/mips.md	(working copy)
@@ -134,10 +134,14 @@ 
   ;; Used in a call expression in place of args_size.  It's present for PIC
   ;; indirect calls where it contains args_size and the function symbol.
   UNSPEC_CALL_ATTR
+
+  ;; MIPS16 casesi jump table dispatch.
+  UNSPEC_CASESI_DISPATCH
 ])
 
 (define_constants
   [(TLS_GET_TP_REGNUM		3)
+   (MIPS16_T_REGNUM		24)
    (PIC_FUNCTION_ADDR_REGNUM	25)
    (RETURN_ADDR_REGNUM		31)
    (CPRESTORE_SLOT_REGNUM	76)
@@ -5904,14 +5908,9 @@ 
   [(set (pc)
 	(match_operand 0 "register_operand"))
    (use (label_ref (match_operand 1 "")))]
-  ""
+  "!TARGET_MIPS16_SHORT_JUMP_TABLES"
 {
-  if (TARGET_MIPS16_SHORT_JUMP_TABLES)
-    operands[0] = expand_binop (Pmode, add_optab,
-				convert_to_mode (Pmode, operands[0], false),
-				gen_rtx_LABEL_REF (Pmode, operands[1]),
-				0, 0, OPTAB_WIDEN);
-  else if (TARGET_GPWORD)
+  if (TARGET_GPWORD)
     operands[0] = expand_binop (Pmode, add_optab, operands[0],
 				pic_offset_table_rtx, 0, 0, OPTAB_WIDEN);
   else if (TARGET_RTP_PIC)
@@ -5937,6 +5936,94 @@ 
   [(set_attr "type" "jump")
    (set_attr "mode" "none")])
 
+;; For MIPS16, we don't know whether a given jump table will use short or
+;; word-sized offsets until late in compilation, when we are able to determine
+;; the sizes of the insns which comprise the containing function.  This
+;; necessitates the use of the casesi rather than the tablejump pattern, since
+;; the latter tries to calculate the index of the offset to jump through early
+;; in compilation, i.e. at expand time, when nothing is known about the
+;; eventual function layout.
+
+(define_expand "casesi"
+  [(match_operand:SI 0 "register_operand" "")	; index to jump on
+   (match_operand:SI 1 "const_int_operand" "")	; lower bound
+   (match_operand:SI 2 "const_int_operand" "")	; total range
+   (match_operand 3 "" "")			; table label
+   (match_operand 4 "" "")]			; out of range label
+  "TARGET_MIPS16_SHORT_JUMP_TABLES"
+{
+  if (operands[1] != const0_rtx)
+    {
+      rtx reg = gen_reg_rtx (SImode);
+      rtx offset = gen_int_mode (-INTVAL (operands[1]), SImode);
+      
+      if (!arith_operand (offset, SImode))
+        offset = force_reg (SImode, offset);
+      
+      emit_insn (gen_addsi3 (reg, operands[0], offset));
+      operands[0] = reg;
+    }
+
+  if (!arith_operand (operands[0], SImode))
+    operands[0] = force_reg (SImode, operands[0]);
+
+  operands[2] = GEN_INT (INTVAL (operands[2]) + 1);
+
+  emit_jump_insn (PMODE_INSN (gen_casesi_internal_mips16,
+			      (operands[0], operands[2],
+			       operands[3], operands[4])));
+
+  DONE;
+})
+
+(define_insn "casesi_internal_mips16_<mode>"
+  [(set (pc)
+     (if_then_else
+       (leu (match_operand:SI 0 "register_operand" "d")
+	    (match_operand:SI 1 "arith_operand" "dI"))
+       (unspec:P
+        [(match_dup 0)
+	 (label_ref (match_operand 2 "" ""))]
+	UNSPEC_CASESI_DISPATCH)
+       (label_ref (match_operand 3 "" ""))))
+   (clobber (match_scratch:P 4 "=d"))
+   (clobber (match_scratch:P 5 "=d"))
+   (clobber (reg:SI MIPS16_T_REGNUM))]
+  "TARGET_MIPS16_SHORT_JUMP_TABLES"
+{
+  rtx diff_vec = PATTERN (next_real_insn (operands[2]));
+
+  gcc_assert (GET_CODE (diff_vec) == ADDR_DIFF_VEC);
+  
+  output_asm_insn ("sltu\t%0, %1", operands);
+  output_asm_insn ("bteqz\t%3", operands);
+  
+  switch (GET_MODE (diff_vec))
+    {
+    case HImode:
+      output_asm_insn ("sll\t%5, %0, 1", operands);
+      output_asm_insn ("la\t%4, %2", operands);
+      output_asm_insn ("<d>addu\t%5, %4, %5", operands);
+      output_asm_insn ("lh\t%5, 0(%5)", operands);
+      break;
+    
+    case SImode:
+      output_asm_insn ("sll\t%5, %0, 2", operands);
+      output_asm_insn ("la\t%4, %2", operands);
+      output_asm_insn ("<d>addu\t%5, %4, %5", operands);
+      output_asm_insn ("lw\t%5, 0(%5)", operands);
+      break;
+
+    default:
+      gcc_unreachable ();
+    }
+  
+  output_asm_insn ("addu\t%4, %4, %5", operands);
+  
+  return "j\t%4";
+}
+  [(set_attr "length" "32")])
+
 ;; For TARGET_USE_GOT, we save the gp in the jmp_buf as well.
 ;; While it is possible to either pull it off the stack (in the
 ;; o32 case) or recalculate it given t9 and our target label,
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 190463)
+++ gcc/config/mips/mips.c	(working copy)
@@ -15575,7 +15575,8 @@  mips16_split_long_branches (void)
       for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
 	if (JUMP_P (insn)
 	    && USEFUL_INSN_P (insn)
-	    && get_attr_length (insn) > 8)
+	    && get_attr_length (insn) > 8
+	    && (any_condjump_p (insn) || any_uncondjump_p (insn)))
 	  {
 	    rtx old_label, new_label, temp, saved_temp;
 	    rtx target, jump, jump_sequence;
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(revision 190463)
+++ gcc/config/mips/mips.h	(working copy)
@@ -2330,13 +2330,18 @@  typedef struct mips_args {
 /* True if we're generating a form of MIPS16 code in which jump tables
    are stored in the text section and encoded as 16-bit PC-relative
    offsets.  This is only possible when general text loads are allowed,
-   since the table access itself will be an "lh" instruction.  */
-/* ??? 16-bit offsets can overflow in large functions.  */
+   since the table access itself will be an "lh" instruction.  If the
+   PC-relative offsets grow too large, 32-bit offsets are used instead.  */
 #define TARGET_MIPS16_SHORT_JUMP_TABLES TARGET_MIPS16_TEXT_LOADS
 
 #define JUMP_TABLES_IN_TEXT_SECTION TARGET_MIPS16_SHORT_JUMP_TABLES
 
-#define CASE_VECTOR_MODE (TARGET_MIPS16_SHORT_JUMP_TABLES ? HImode : ptr_mode)
+#define CASE_VECTOR_MODE SImode
+
+/* Only use short offsets if their range will not overflow.  */
+#define CASE_VECTOR_SHORTEN_MODE(MIN, MAX, BODY) \
+  (TARGET_MIPS16_SHORT_JUMP_TABLES && ((MIN) >= -32768 && (MAX) < 32768) \
+   ? HImode : SImode)
 
 #define CASE_VECTOR_PC_RELATIVE TARGET_MIPS16_SHORT_JUMP_TABLES
 
@@ -2636,8 +2641,14 @@  while (0)
 #define ASM_OUTPUT_ADDR_DIFF_ELT(STREAM, BODY, VALUE, REL)		\
 do {									\
   if (TARGET_MIPS16_SHORT_JUMP_TABLES)					\
-    fprintf (STREAM, "\t.half\t%sL%d-%sL%d\n",				\
-	     LOCAL_LABEL_PREFIX, VALUE, LOCAL_LABEL_PREFIX, REL);	\
+    {									\
+      if (GET_MODE (BODY) == HImode)					\
+	fprintf (STREAM, "\t.half\t%sL%d-%sL%d\n",			\
+		 LOCAL_LABEL_PREFIX, VALUE, LOCAL_LABEL_PREFIX, REL);	\
+      else								\
+	fprintf (STREAM, "\t.word\t%sL%d-%sL%d\n",			\
+		 LOCAL_LABEL_PREFIX, VALUE, LOCAL_LABEL_PREFIX, REL);	\
+    }									\
   else if (TARGET_GPWORD)						\
     fprintf (STREAM, "\t%s\t%sL%d\n",					\
 	     ptr_mode == DImode ? ".gpdword" : ".gpword",		\
Index: gcc/testsuite/gcc.target/mips/code-readable-1.c
===================================================================
--- gcc/testsuite/gcc.target/mips/code-readable-1.c	(revision 190463)
+++ gcc/testsuite/gcc.target/mips/code-readable-1.c	(working copy)
@@ -1,4 +1,4 @@ 
-/* { dg-options "(-mips16) -mcode-readable=yes -mgp32 addressing=absolute" } */
+/* { dg-options "(-mips16) -mcode-readable=yes -mgp32 addressing=absolute -O" } */
 
 MIPS16 int
 foo (int i)