[PATCH, MIPS] fix MIPS16 jump table overflow

[PATCH, MIPS] fix MIPS16 jump table overflow

[Sandra Loosemore]

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

In config/mips/mips.h, there is presently this comment:

/* ??? 16-bit offsets can overflow in large functions.  */
#define TARGET_MIPS16_SHORT_JUMP_TABLES TARGET_MIPS16_TEXT_LOADS

A while ago we had a bug report where a big switch statement did, in 
fact, overflow the range of 16-bit offsets, causing a runtime error.

GCC already has generic machinery to shorten offset tables for switch 
statements that does the necessary range checking, but it only works 
with "casesi", not the lower-level "tablejump" expansion.  So, this 
patch provides a "casesi" expander to handle this situation.

This patch has been in use on our local 4.5 and 4.6 branches for about a 
year now.  When testing it on mainline, I found it tripped over the 
recent change to add MIPS16 branch overflow checking in other 
situations, causing it to get into an infinite loop.  I think telling it 
to ignore these new jump insns it doesn't know how to process is the 
right thing to do, but I'm not sure if there's a better way to restrict 
the condition or make mips16_split_long_branches more robust.  Richard,
since that's your code I assume you'll suggest an alternative if this 
doesn't meet with your approval.  Is the rest of the patch OK to check in?

-Sandra


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

	gcc/
	* config/mips/mips.md (MIPS16_T_REGNUM): New constant.
	(tablejump): Don't use for MIPS16_SHORT_JUMP_TABLES.
	(casesi): New.
	(casesi_internal_mips16): New.
	* config/mips/mips.c (mips16_split_long_branches): Ignore
	casesi_internal_mips16 insns.
	* config/mips/mips.h (TARGET_MIPS16_SHORT_JUMP_TABLES): Update
	comment.
	(CASE_VECTOR_MODE): Use ptr_mode 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: Generalize assembler
	pattern	for jump table.

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

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	(revision 190463)
+++ gcc/config/mips/mips.md	(working copy)
@@ -138,6 +138,7 @@
 
 (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 +5905,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 +5933,91 @@
   [(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:SI 3 "" "")			; table label
+   (match_operand:SI 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 (gen_casesi_internal_mips16 (operands[0], operands[2],
+					      operands[3], operands[4]));
+
+  DONE;
+})
+
+(define_insn "casesi_internal_mips16"
+  [(set (pc)
+     (if_then_else
+       (leu (match_operand:SI 0 "register_operand" "d")
+	    (match_operand:SI 1 "arith_operand" "dI"))
+       (mem:SI (plus:SI (mult:SI (match_dup 0) (const_int 4))
+			(label_ref (match_operand 2 "" ""))))
+       (label_ref (match_operand 3 "" ""))))
+   (clobber (match_scratch:SI 4 "=&d"))
+   (clobber (match_scratch:SI 5 "=d"))
+   (clobber (reg:SI MIPS16_T_REGNUM))
+   (use (label_ref (match_dup 2)))]
+  "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);
+  output_asm_insn ("la\t%4, %2", operands);
+  
+  switch (GET_MODE (diff_vec))
+    {
+    case HImode:
+      output_asm_insn ("sll\t%5, %0, 1", operands);
+      output_asm_insn ("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 ("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
+	    && INSN_CODE (insn) != CODE_FOR_casesi_internal_mips16)
 	  {
 	    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 ptr_mode
+
+/* 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 : ptr_mode)
 
 #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)
@@ -25,7 +25,7 @@ bar (void)
 }
 
 /* { dg-final { scan-assembler "\tla\t" } } */
-/* { dg-final { scan-assembler "\t\\.half\t" } } */
+/* { dg-final { scan-assembler "\t\\.\(half\|word\)\t\\.L\[0-9\]*-\\.L\[0-9\]\*" } } */
 /* { dg-final { scan-assembler-not "%hi\\(\[^)\]*L" } } */
 /* { dg-final { scan-assembler-not "%lo\\(\[^)\]*L" } } */
 

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Richard Sandiford]

Sandra Loosemore <sandra@codesourcery.com> writes:
> In config/mips/mips.h, there is presently this comment:
>
> /* ??? 16-bit offsets can overflow in large functions.  */
> #define TARGET_MIPS16_SHORT_JUMP_TABLES TARGET_MIPS16_TEXT_LOADS
>
> A while ago we had a bug report where a big switch statement did, in 
> fact, overflow the range of 16-bit offsets, causing a runtime error.
>
> GCC already has generic machinery to shorten offset tables for switch 
> statements that does the necessary range checking, but it only works 
> with "casesi", not the lower-level "tablejump" expansion.  So, this 
> patch provides a "casesi" expander to handle this situation.

Nice.

> This patch has been in use on our local 4.5 and 4.6 branches for about a 
> year now.  When testing it on mainline, I found it tripped over the 
> recent change to add MIPS16 branch overflow checking in other 
> situations, causing it to get into an infinite loop.  I think telling it 
> to ignore these new jump insns it doesn't know how to process is the 
> right thing to do, but I'm not sure if there's a better way to restrict 
> the condition or make mips16_split_long_branches more robust.  Richard,
> since that's your code I assume you'll suggest an alternative if this 
> doesn't meet with your approval.

Changing it to:

	if (JUMP_P (insn)
	    && USEFUL_INSN_P (insn)
	    && get_attr_length (insn) > 8
	    && (any_condjump_p (insn) || any_uncond_jump_p (insn)))

should be OK.

> @@ -5937,6 +5933,91 @@
>    [(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:SI 3 "" "")			; table label
> +   (match_operand:SI 4 "" "")]			; out of range label

The last two are Pmode rather than SImode.  Since there aren't different
case* patterns for different Pmodes, we can't use :P instead, so let's just
drop the modes on 4 and 5.

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.

> +(define_insn "casesi_internal_mips16"
> +  [(set (pc)
> +     (if_then_else
> +       (leu (match_operand:SI 0 "register_operand" "d")
> +	    (match_operand:SI 1 "arith_operand" "dI"))
> +       (mem:SI (plus:SI (mult:SI (match_dup 0) (const_int 4))
> +			(label_ref (match_operand 2 "" ""))))
> +       (label_ref (match_operand 3 "" ""))))
> +   (clobber (match_scratch:SI 4 "=&d"))
> +   (clobber (match_scratch:SI 5 "=d"))
> +   (clobber (reg:SI MIPS16_T_REGNUM))
> +   (use (label_ref (match_dup 2)))]

Although this is descriptive, the MEM is probably more trouble
than it's worth.  A hard-coded MEM like this will alias a lot
of things, whereas we're only reading from the function itself.
I think an unspec would be better.

This pattern should have :P for operands 4 and 5, with the pattern
name becoming:

"casesi_internal_mips16_<mode>"

PMODE_INSN should make it easy to wrap up the difference.

There shouldn't be any need for the final USE.  Let me know
if you found otherwise, because that sounds like a bug.

> +  "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);
> +  output_asm_insn ("la\t%4, %2", operands);
> +  
> +  switch (GET_MODE (diff_vec))
> +    {
> +    case HImode:
> +      output_asm_insn ("sll\t%5, %0, 1", operands);
> +      output_asm_insn ("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 ("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")])

The "addu"s here ought to be "<d>addu"s after the :P change.

I think we can avoid the earlyclobber on operand 4 by moving the LA
after the SLL.

> +#define CASE_VECTOR_MODE ptr_mode
> +
> +/* 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 : ptr_mode)

ptr_mode should be SImode here, to match the switch above.
We don't even begin to support functions bigger than 2GB. :-)

> 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)
> @@ -25,7 +25,7 @@ bar (void)
>  }
>  
>  /* { dg-final { scan-assembler "\tla\t" } } */
> -/* { dg-final { scan-assembler "\t\\.half\t" } } */
> +/* { dg-final { scan-assembler "\t\\.\(half\|word\)\t\\.L\[0-9\]*-\\.L\[0-9\]\*" } } */
>  /* { dg-final { scan-assembler-not "%hi\\(\[^)\]*L" } } */
>  /* { dg-final { scan-assembler-not "%lo\\(\[^)\]*L" } } */
>  

I'd prefer to add -O and stick to requiring .half.

We call shorten_branches several times, so I was worried that we
might be shortening the cases prematurely.  It looks like later
calls are already able to undo the shortening though, so hopefully
that isn't a problem.

Richard

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Sandra Loosemore]

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

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.


[-- Attachment #2: jumptable-new.patch --]
[-- Type: text/x-patch, Size: 7409 bytes --]

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)

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Richard Sandiford]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Richard Sandiford]

Sandra Loosemore <sandra@codesourcery.com> writes:
> 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.

Yeah, -mabi=64 isn't supported, because the associated relocs haven't
been defined.  It has to be EABI64.

I committed the test case below after checking that it passed on
mips64-elf after your changes.  Nice to see that it worked first time!

Richard


gcc/testsuite/
	* gcc.target/mips/code-readable-4.c: New test.

Index: gcc/testsuite/gcc.target/mips/code-readable-4.c
===================================================================
--- /dev/null	2012-08-19 20:42:12.842999468 +0100
+++ gcc/testsuite/gcc.target/mips/code-readable-4.c	2012-08-23 21:38:12.956961995 +0100
@@ -0,0 +1,34 @@
+/* { dg-options "(-mips16) -mcode-readable=yes -mabi=eabi -mgp64 -O" } */
+
+MIPS16 int
+foo (int i)
+{
+  switch (i)
+    {
+    case 1: return 40;
+    case 2: return 11;
+    case 3: return 29;
+    case 4: return 10;
+    case 5: return 12;
+    case 6: return 35;
+    case 7: return 23;
+    default: return 0;
+    }
+}
+
+extern int k[];
+
+MIPS16 int *
+bar (void)
+{
+  return k;
+}
+
+/* { dg-final { scan-assembler "\tla\t" } } */
+/* { dg-final { scan-assembler "\t\\.half\t" } } */
+/* { dg-final { scan-assembler-not "%hi\\(\[^)\]*L" } } */
+/* { dg-final { scan-assembler-not "%lo\\(\[^)\]*L" } } */
+
+/* { dg-final { scan-assembler "\t\\.dword\tk\n" } } */
+/* { dg-final { scan-assembler-not "%hi\\(k\\)" } } */
+/* { dg-final { scan-assembler-not "%lo\\(k\\)" } } */

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Andrew Pinski]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Andrew Pinski]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Richard Sandiford]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Andrew Pinski]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Sandra Loosemore]

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

Re: [PATCH, MIPS] fix MIPS16 jump table overflow

[Andrew Pinski]

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

On Sat, Aug 25, 2012 at 8:52 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> 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.


Here is the patch which I applied after a bootstrap/test on mips64-linux-gnu.

Thanks,
Andrew Pinski

ChangeLog:
* config/mips/mips.h (CASE_VECTOR_MODE): For not
TARGET_MIPS16_SHORT_JUMP_TABLES use ptr_mode.
(CASE_VECTOR_SHORTEN_MODE): Likewise.

[-- Attachment #2: fixmips.diff.txt --]
[-- Type: text/plain, Size: 813 bytes --]

Index: config/mips/mips.h
===================================================================
--- config/mips/mips.h	(revision 190638)
+++ config/mips/mips.h	(working copy)
@@ -2336,12 +2336,13 @@ typedef struct mips_args {
 
 #define JUMP_TABLES_IN_TEXT_SECTION TARGET_MIPS16_SHORT_JUMP_TABLES
 
-#define CASE_VECTOR_MODE SImode
+#define CASE_VECTOR_MODE (TARGET_MIPS16_SHORT_JUMP_TABLES ? SImode : ptr_mode)
 
 /* 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)
+  (!TARGET_MIPS16_SHORT_JUMP_TABLES ? ptr_mode \
+   : ((MIN) >= -32768 && (MAX) < 32768) ? HImode \
+   : SImode)
 
 #define CASE_VECTOR_PC_RELATIVE TARGET_MIPS16_SHORT_JUMP_TABLES