[PATCH] microMIPS/GCC: Correct 64-bit instruction sizes

[PATCH] microMIPS/GCC: Correct 64-bit instruction sizes

[Maciej W. Rozycki]

Hi,

 Noticed in an attempt to build a 64-bit microMIPS Linux kernel.

 None of the 64-bit operations have a short instruction encoding in the 
microMIPS instruction set.  Despite that our code has been written such 
that the presence of a short encoding is assumed for both 32-bit and 
64-bit operations.  This leads to assembly warnings like:

{standard input}: Assembler messages:
{standard input}:146: Warning: wrong size instruction in a 16-bit branch delay slot
{standard input}:544: Warning: wrong size instruction in a 16-bit branch delay slot
{standard input}:1071: Warning: wrong size instruction in a 16-bit branch delay slot
{standard input}:1088: Warning: wrong size instruction in a 16-bit branch delay slot
{standard input}:1833: Warning: wrong size instruction in a 16-bit branch delay slot

This is because code in question produced by GCC looks like:

	jals	get_option	 #
	daddiu	$4,$sp,16	 #,,

(in a `.set noreorder' fragment) and there is no short encoding for the 
DADDIU instruction available.

 At least two approaches are possible to address this problem, either by 
splitting the iterated patterns affected into separate 32-bit and 64-bit 
patterns, or by limiting the affected alternatives to 32-bit operations 
only.  I found the latter less intrusive, with the use of an extra 
`compression' attribute value: `micromips32'.  This is implemented with 
the change below, fixing the issues seen in 64-bit compilation.  Code 
produced now looks like:

	jal	get_option	 #
	daddiu	$4,$sp,16	 #,,

instead.

 Some operations are operand size agnostic, these include LI16, MOVE16, 
and all the relevant logical instructions.  These retain the `micromips' 
setting for the `compression' attribute.  MOVEP is also operand size 
agnostic, but it must not be scheduled into a delay slot and it is 
therefore handled separately, with no `compression' attribute defined.

 Regression-tested with the mips-linux-gnu target and these multilibs:

-EB
-EB -mips16
-EB -mmicromips
-EB -mabi=n32
-EB -mabi=64

and the -EL variants of same, with no changes in results.  I have also 
checked that microMIPS shared glibc libraries have not changed when 
rebuilt with the updated compiler.

 OK to apply?

2014-11-14  Maciej W. Rozycki  <macro@codesourcery.com>

	gcc/
	* config/mips/mips.md (compression): Add `micromips32' setting.
	(enabled, length): Handle it.
	(shift_compression): Replace `micromips' with `micromips32' in
	the `compression' attribute.
	(*add<mode>3, sub<mode>3): Likewise.

  Maciej

gcc-umips32-compression.diff
Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.md
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.md	2014-11-14 18:34:25.000000000 +0000
+++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.md	2014-11-14 20:56:20.278971795 +0000
@@ -429,7 +429,7 @@
 		(const_string "yes")
 		(const_string "no")))
 
-(define_attr "compression" "none,all,micromips"
+(define_attr "compression" "none,all,micromips32,micromips"
   (const_string "none"))
 
 (define_attr "enabled" "no,yes"
@@ -440,7 +440,7 @@
 			   || TARGET_O32_FP64A_ABI")
 	      (eq_attr "dword_mode" "yes"))
 	 (const_string "no")
-	 (and (eq_attr "compression" "micromips")
+	 (and (eq_attr "compression" "micromips32,micromips")
 	      (match_test "!TARGET_MICROMIPS"))
 	 (const_string "no")]
 	(const_string "yes")))
@@ -526,7 +526,9 @@
 ;; but there are special cases for branches (which must be handled here)
 ;; and for compressed single instructions.
 (define_attr "length" ""
-   (cond [(and (eq_attr "compression" "micromips,all")
+   (cond [(and (ior (eq_attr "compression" "micromips,all")
+		    (and (eq_attr "compression" "micromips32")
+			 (eq_attr "mode" "SI,SF")))
 	       (eq_attr "dword_mode" "no")
 	       (match_test "TARGET_MICROMIPS"))
 	  (const_int 2)
@@ -979,8 +981,8 @@
 				  (xor "xori")
 				  (and "andi")])
 
-(define_code_attr shift_compression [(ashift "micromips")
-				     (lshiftrt "micromips")
+(define_code_attr shift_compression [(ashift "micromips32")
+				     (lshiftrt "micromips32")
 				     (ashiftrt "none")])
 
 ;; <fcond> is the c.cond.fmt condition associated with a particular code.
@@ -1163,7 +1165,7 @@
     return "<d>addiu\t%0,%1,%2";
 }
   [(set_attr "alu_type" "add")
-   (set_attr "compression" "micromips,*,micromips,micromips,micromips,micromips,*")
+   (set_attr "compression" "micromips32,*,micromips32,micromips32,micromips32,micromips32,*")
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*add<mode>3_mips16"
@@ -1381,7 +1383,7 @@
   ""
   "<d>subu\t%0,%1,%2"
   [(set_attr "alu_type" "sub")
-   (set_attr "compression" "micromips,*")
+   (set_attr "compression" "micromips32,*")
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*subsi3_extended"

RE: [PATCH] microMIPS/GCC: Correct 64-bit instruction sizes

[Moore, Catherine]

Hi Maciej,

> -----Original Message-----
> From: Rozycki, Maciej
> Sent: Monday, November 17, 2014 2:39 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Moore, Catherine; Eric Christopher; Matthew Fortune
> Subject: [PATCH] microMIPS/GCC: Correct 64-bit instruction sizes
> 
> Hi,
> 
>  Noticed in an attempt to build a 64-bit microMIPS Linux kernel.
> 
>  None of the 64-bit operations have a short instruction encoding in the
> microMIPS instruction set.  Despite that our code has been written such that
> the presence of a short encoding is assumed for both 32-bit and 64-bit
> operations.  This leads to assembly warnings like:
> 
> {standard input}: Assembler messages:
> {standard input}:146: Warning: wrong size instruction in a 16-bit branch delay
> slot {standard input}:544: Warning: wrong size instruction in a 16-bit branch
> delay slot {standard input}:1071: Warning: wrong size instruction in a 16-bit
> branch delay slot {standard input}:1088: Warning: wrong size instruction in a
> 16-bit branch delay slot {standard input}:1833: Warning: wrong size
> instruction in a 16-bit branch delay slot
> 
> This is because code in question produced by GCC looks like:
> 
> 	jals	get_option	 #
> 	daddiu	$4,$sp,16	 #,,
> 
> (in a `.set noreorder' fragment) and there is no short encoding for the
> DADDIU instruction available.
> 
>  At least two approaches are possible to address this problem, either by
> splitting the iterated patterns affected into separate 32-bit and 64-bit
> patterns, or by limiting the affected alternatives to 32-bit operations only.  I
> found the latter less intrusive, with the use of an extra `compression'
> attribute value: `micromips32'.  

I prefer the second approach as well.

This is implemented with the change below,
> fixing the issues seen in 64-bit compilation.  Code produced now looks like:
> 
> 	jal	get_option	 #
> 	daddiu	$4,$sp,16	 #,,
> 
> instead.
> 
>  Some operations are operand size agnostic, these include LI16, MOVE16, and
> all the relevant logical instructions.  These retain the `micromips'
> setting for the `compression' attribute.  MOVEP is also operand size agnostic,
> but it must not be scheduled into a delay slot and it is therefore handled
> separately, with no `compression' attribute defined.
> 
>  Regression-tested with the mips-linux-gnu target and these multilibs:
> 
> -EB
> -EB -mips16
> -EB -mmicromips
> -EB -mabi=n32
> -EB -mabi=64
> 
> and the -EL variants of same, with no changes in results.  I have also checked
> that microMIPS shared glibc libraries have not changed when rebuilt with the
> updated compiler.
> 
>  OK to apply?
> 
> 2014-11-14  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gcc/
> 	* config/mips/mips.md (compression): Add `micromips32' setting.
> 	(enabled, length): Handle it.
> 	(shift_compression): Replace `micromips' with `micromips32' in
> 	the `compression' attribute.
> 	(*add<mode>3, sub<mode>3): Likewise.

Yes, this looks good.
Thanks,
Catherine

RE: [PATCH] microMIPS/GCC: Correct 64-bit instruction sizes

[Maciej W. Rozycki]

On Mon, 17 Nov 2014, Moore, Catherine wrote:

> > 	* config/mips/mips.md (compression): Add `micromips32' setting.
> > 	(enabled, length): Handle it.
> > 	(shift_compression): Replace `micromips' with `micromips32' in
> > 	the `compression' attribute.
> > 	(*add<mode>3, sub<mode>3): Likewise.
> 
> Yes, this looks good.

 Applied now, thanks for your review.

  Maciej