diff mbox

[committed] PR 53916: divmod not effective for MIPS16

Message ID 87li73jbhb.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford May 25, 2013, 4 p.m. UTC 
PR 53916 was originally about a wrong-code division bug in 4.6 and earlier.
Although that in itself has been fixed, the reported pointed out that the
new code isn't able to reuse the same MIPS16 instruction for both division
and modulus results.  The problem is that, for MIPS16, we now expose the
(fixed) LO register from the outset, which stops the rtl optimisers
from reusing the result of an earlier instruction.

As explained in the comment, this patch hides the LO register until
after CSE, but still tries to expose it before register allocation.

Tested on mips64-linux-gnu and applied.

Richard


gcc/
	PR target/53916
	* config/mips/constraints.md (kl): New constraint.
	* config/mips/mips.md (divmod<mode>4, udivmod<mode>4): Delete.
	(divmod<mode>4_internal): Rename to divmod<mode>4.  Use "kl" as the
	constraint for operand 0.  Split after CSE for MIPS16.  Emit a move
	from LO for MIPS16.
	(udivmod<mode>4_internal): Likewise udivmod<mode>4.

gcc/testsuite/
	PR target/53916
	* gcc.target/mips/div-13.c: New test.
diff mbox

Patch

Index: gcc/config/mips/constraints.md
===================================================================
--- gcc/config/mips/constraints.md	2013-05-25 12:15:32.050687848 +0100
+++ gcc/config/mips/constraints.md	2013-05-25 12:19:29.723476968 +0100
@@ -92,6 +92,12 @@  (define_register_constraint "D" "COP3_RE
 ;; but the DSP version allows any accumulator target.
 (define_register_constraint "ka" "ISA_HAS_DSP_MULT ? ACC_REGS : MD_REGS")
 
+;; The register class to use for an allocatable division result.
+;; MIPS16 uses M16_REGS because LO is fixed.
+(define_register_constraint "kl"
+  "TARGET_MIPS16 ? M16_REGS : TARGET_BIG_ENDIAN ? MD1_REG : MD0_REG"
+  "@internal")
+
 (define_constraint "kf"
   "@internal"
   (match_operand 0 "force_to_mem_operand"))
Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	2013-05-25 12:14:10.900077754 +0100
+++ gcc/config/mips/mips.md	2013-05-25 12:15:14.393555017 +0100
@@ -2560,80 +2560,50 @@  (define_insn "*recip<mode>3"
 
 ;; VR4120 errata MD(A1): signed division instructions do not work correctly
 ;; with negative operands.  We use special libgcc functions instead.
-(define_expand "divmod<mode>4"
-  [(set (match_operand:GPR 0 "register_operand")
-	(div:GPR (match_operand:GPR 1 "register_operand")
-		 (match_operand:GPR 2 "register_operand")))
-   (set (match_operand:GPR 3 "register_operand")
-	(mod:GPR (match_dup 1)
-		 (match_dup 2)))]
-  "!TARGET_FIX_VR4120"
-{
-  if (TARGET_MIPS16)
-    {
-      emit_insn (gen_divmod<mode>4_split (operands[3], operands[1],
-					  operands[2]));
-      emit_move_insn (operands[0], gen_rtx_REG (<MODE>mode, LO_REGNUM));
-    }
-  else
-    emit_insn (gen_divmod<mode>4_internal (operands[0], operands[1],
-					   operands[2], operands[3]));
-  DONE;
-})
-
-(define_insn_and_split "divmod<mode>4_internal"
-  [(set (match_operand:GPR 0 "muldiv_target_operand" "=l")
+;;
+;; Expand generates divmod instructions for individual division and modulus
+;; operations.  We then rely on CSE to reuse earlier divmods where possible.
+;; This means that, when generating MIPS16 code, it is better not to expose
+;; the fixed LO register until after CSE has finished.  However, it's still
+;; better to split before register allocation, so that we don't allocate
+;; one of the scarce MIPS16 registers to an unused result.
+(define_insn_and_split "divmod<mode>4"
+  [(set (match_operand:GPR 0 "register_operand" "=kl")
 	(div:GPR (match_operand:GPR 1 "register_operand" "d")
 		 (match_operand:GPR 2 "register_operand" "d")))
    (set (match_operand:GPR 3 "register_operand" "=d")
 	(mod:GPR (match_dup 1)
 		 (match_dup 2)))]
-  "!TARGET_FIX_VR4120 && !TARGET_MIPS16"
+  "!TARGET_FIX_VR4120"
   "#"
-  "&& reload_completed"
+  "&& ((TARGET_MIPS16 && cse_not_expected) || reload_completed)"
   [(const_int 0)]
 {
   emit_insn (gen_divmod<mode>4_split (operands[3], operands[1], operands[2]));
+  if (TARGET_MIPS16)
+    emit_move_insn (operands[0], gen_rtx_REG (<MODE>mode, LO_REGNUM));
   DONE;
 }
  [(set_attr "type" "idiv")
   (set_attr "mode" "<MODE>")
   (set_attr "length" "8")])
 
-(define_expand "udivmod<mode>4"
-  [(set (match_operand:GPR 0 "register_operand")
-	(udiv:GPR (match_operand:GPR 1 "register_operand")
-		  (match_operand:GPR 2 "register_operand")))
-   (set (match_operand:GPR 3 "register_operand")
-	(umod:GPR (match_dup 1)
-		  (match_dup 2)))]
-  ""
-{
-  if (TARGET_MIPS16)
-    {
-      emit_insn (gen_udivmod<mode>4_split (operands[3], operands[1],
-					   operands[2]));
-      emit_move_insn (operands[0], gen_rtx_REG (<MODE>mode, LO_REGNUM));
-    }
-  else
-    emit_insn (gen_udivmod<mode>4_internal (operands[0], operands[1],
-					    operands[2], operands[3]));
-  DONE;
-})
-
-(define_insn_and_split "udivmod<mode>4_internal"
-  [(set (match_operand:GPR 0 "muldiv_target_operand" "=l")
+;; See the comment above "divmod<mode>4" for the MIPS16 handling.
+(define_insn_and_split "udivmod<mode>4"
+  [(set (match_operand:GPR 0 "register_operand" "=kl")
 	(udiv:GPR (match_operand:GPR 1 "register_operand" "d")
 		  (match_operand:GPR 2 "register_operand" "d")))
    (set (match_operand:GPR 3 "register_operand" "=d")
 	(umod:GPR (match_dup 1)
 		  (match_dup 2)))]
-  "!TARGET_MIPS16"
+  ""
   "#"
-  "reload_completed"
+  "(TARGET_MIPS16 && cse_not_expected) || reload_completed"
   [(const_int 0)]
 {
   emit_insn (gen_udivmod<mode>4_split (operands[3], operands[1], operands[2]));
+  if (TARGET_MIPS16)
+    emit_move_insn (operands[0], gen_rtx_REG (<MODE>mode, LO_REGNUM));
   DONE;
 }
  [(set_attr "type" "idiv")
Index: gcc/testsuite/gcc.target/mips/div-13.c
===================================================================
--- /dev/null	2013-05-20 18:02:16.162117076 +0100
+++ gcc/testsuite/gcc.target/mips/div-13.c	2013-05-25 12:15:14.394555025 +0100
@@ -0,0 +1,17 @@ 
+/* { dg-options "(-mips16) -mgp64" } */
+/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
+
+typedef int int32_t;
+typedef unsigned int uint32_t;
+typedef long long int64_t;
+typedef unsigned long long uint64_t;
+
+MIPS16 int32_t f1 (int32_t x, int32_t y) { return x / y + x % y; }
+MIPS16 uint32_t f2 (uint32_t x, uint32_t y) { return x / y + x % y; }
+MIPS16 int64_t f3 (int64_t x, int64_t y) { return x / y + x % y; }
+MIPS16 uint64_t f4 (uint64_t x, uint64_t y) { return x / y + x % y; }
+
+/* { dg-final { scan-assembler-times "\tdiv\t" 1 } } */
+/* { dg-final { scan-assembler-times "\tdivu\t" 1 } } */
+/* { dg-final { scan-assembler-times "\tddiv\t" 1 } } */
+/* { dg-final { scan-assembler-times "\tddivu\t" 1 } } */