Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction

[Richard Sandiford <richard@codesourcery.com>]

Nick Clifton <nickc@redhat.com> writes:
> Hi Eric, Hi Richard,
>
>   Another customer bug report has pointed out a problem with the MIPS
>   compiler.  Compile the attached test case with:
>
>     mips64vrel-elf-gcc -mgp32 -mlong64 -march=vr5000 -O2 128195.c -S -dp
>
>   (I presume that this problem exists with other MIPS targets apart
>   from the mips64vrel-elf.  It just so happens that this is the
>   toolchain that the customer is using).
>   
>   Look at the assembler output.  In particular:
>
>     ldc1  $f2,0($4)	 # 6	*movdf_hardfloat_32bit/3	[length = 8]
>
>   Note the assumed length of the ldc1 instruction.  In the original
>   bug report this was a problem because the ldc1 instruction was
>   between an mfhi and a mult instruction, and gcc thought that this
>   was sufficient to act as a hilo interlock.  This does not appear to
>   happen with the current mainline gcc sources, but I still think that
>   the incorrect length is a bug.
>
>   Tracking the problem through the code, I found that
>   mips_address_insns() was saying that it takes 2 instructions to
>   implement this rtl:
>
>     (mem:DF (reg:SI $4))
>
>   when in fact it only takes one.  The patch below fixes this, in what
>   I hope is the correct way.  I checked the patch by recompiling a
>   mips64vrel-elf toolchain and running a gcc testsuite regression test
>   for the default multilib and the vr5000/long64/gp32 multilib.
>   - There were no regressions.  I am not sure if this covers all
>   eventualities though.
>[...]
> Index: gcc/config/mips/mips.c
> ===================================================================
> --- gcc/config/mips/mips.c	(revision 127178)
> +++ gcc/config/mips/mips.c	(working copy)
> @@ -1963,6 +1963,9 @@ mips_address_insns (rtx x, enum machine_
>    if (mode == BLKmode)
>      /* BLKmode is used for single unaligned loads and stores.  */
>      factor = 1;
> +  else if (mode == DFmode && TARGET_HARD_FLOAT && TARGET_DOUBLE_FLOAT)
> +    /* Doubles can be accessed by a single instruction when the hardware supports it.  */
> +    factor = 1;
>    else
>      /* Each word of a multi-word value will be accessed individually.  */
>      factor = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;

Unfortunately, this isn't quite right.  DFmode moves still take
2 instructions when the target is a GPR.  For MIPS I, they will
also take 2 instructions when the target is an FPR.

I think instead that mips_fetch_insns should try to prove that
the move does not need to be split, and tell mips_address_insns
about it.  Does the patch below work for you?

(I think the 'R' thing is a bug fix.  'R' is only used in user asms
and is provided for compatibility with older compilers.  I don't think
users will expect us to second-guess whether two separate loads or stores
are being used.)

I admit this isn't pretty, but I can't think of a prettier way off-hand.

Richard


gcc/
	* config/mips/mips-protos.h (mips_address_insns): Add a boolean
	argument.
	(mips_fetch_insns): Delete in favor of...
	(mips_load_store_insns): ...this new function.
	* config/mips/mips.c (mips_address_insns): Add a boolean argument
	to say whether multiword moves _might_ be split.
	(mips_fetch_insns): Delete in favor of...
	(mips_load_store_insns): ...this new function.
	(mips_rtx_costs): Update the call to mips_address_insns.
	(mips_address_cost): Likewise.
	* config/mips/mips.md (length): Use mips_load_store_insns instead
	of mips_fetch_insns.
	* config/mips/constraints.md (R): Use mips_address_insns rather
	than mips_fetch_insns.  Assume that the move never needs to be split.

Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	2007-08-07 09:52:19.000000000 +0100
+++ gcc/config/mips/mips-protos.h	2007-08-07 10:41:24.000000000 +0100
@@ -140,9 +140,9 @@ struct mips16e_save_restore_info;
 extern bool mips_symbolic_constant_p (rtx, enum mips_symbol_type *);
 extern int mips_regno_mode_ok_for_base_p (int, enum machine_mode, int);
 extern bool mips_stack_address_p (rtx, enum machine_mode);
-extern int mips_address_insns (rtx, enum machine_mode);
+extern int mips_address_insns (rtx, enum machine_mode, bool);
 extern int mips_const_insns (rtx);
-extern int mips_fetch_insns (rtx);
+extern int mips_load_store_insns (rtx, rtx);
 extern int mips_idiv_insns (void);
 extern int fp_register_operand (rtx, enum machine_mode);
 extern int lo_operand (rtx, enum machine_mode);
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2007-08-07 09:39:37.000000000 +0100
+++ gcc/config/mips/mips.c	2007-08-07 10:45:29.000000000 +0100
@@ -1950,22 +1950,26 @@ mips16_unextended_reference_p (enum mach
 
 
 /* Return the number of instructions needed to load or store a value
-   of mode MODE at X.  Return 0 if X isn't valid for MODE.
+   of mode MODE at X.  Return 0 if X isn't valid for MODE.  Assume that
+   multiword moves may need to be split into word moves if MIGHT_SPLIT_P,
+   otherwise assume that a single load or store is enough.
 
    For mips16 code, count extended instructions as two instructions.  */
 
 int
-mips_address_insns (rtx x, enum machine_mode mode)
+mips_address_insns (rtx x, enum machine_mode mode, bool might_split_p)
 {
   struct mips_address_info addr;
   int factor;
 
-  if (mode == BLKmode)
-    /* BLKmode is used for single unaligned loads and stores.  */
-    factor = 1;
-  else
-    /* Each word of a multi-word value will be accessed individually.  */
+  /* BLKmode is used for single unaligned loads and stores and should
+     not count as a multiword mode.  (GET_MODE_SIZE (BLKmode) is pretty
+     meaningless, so we have to single it out as a special case one way
+     or the other.)  */
+  if (mode != BLKmode && might_split_p)
     factor = (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
+  else
+    factor = 1;
 
   if (mips_classify_address (&addr, x, mode, false))
     switch (addr.type)
@@ -2061,14 +2065,30 @@ mips_const_insns (rtx x)
 }
 
 
-/* Return the number of instructions needed for memory reference X.
-   Count extended mips16 instructions as two instructions.  */
+/* Return the number of instructions needed to implement INSN,
+   given that it loads from or stores to MEM.  Count extended
+   mips16 instructions as two instructions.  */
 
 int
-mips_fetch_insns (rtx x)
+mips_load_store_insns (rtx mem, rtx insn)
 {
-  gcc_assert (MEM_P (x));
-  return mips_address_insns (XEXP (x, 0), GET_MODE (x));
+  enum machine_mode mode;
+  bool might_split_p;
+  rtx set;
+
+  gcc_assert (MEM_P (mem));
+  mode = GET_MODE (mem);
+
+  /* Try to prove that INSN does not need to be split.  */
+  might_split_p = true;
+  if (GET_MODE_BITSIZE (mode) == 64)
+    {
+      set = single_set (insn);
+      if (set && !mips_split_64bit_move_p (SET_DEST (set), SET_SRC (set)))
+	might_split_p = false;
+    }
+
+  return mips_address_insns (XEXP (mem, 0), mode, might_split_p);
 }
 
 
@@ -2818,7 +2838,7 @@ mips_rtx_costs (rtx x, int code, int out
 	/* If the address is legitimate, return the number of
 	   instructions it needs.  */
 	rtx addr = XEXP (x, 0);
-	int n = mips_address_insns (addr, GET_MODE (x));
+	int n = mips_address_insns (addr, GET_MODE (x), true);
 	if (n > 0)
 	  {
 	    *total = COSTS_N_INSNS (n + 1);
@@ -2973,7 +2993,7 @@ mips_rtx_costs (rtx x, int code, int out
 static int
 mips_address_cost (rtx addr)
 {
-  return mips_address_insns (addr, SImode);
+  return mips_address_insns (addr, SImode, false);
 }
 \f
 /* Return one word of double-word value OP, taking into account the fixed
Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	2007-08-07 09:53:28.000000000 +0100
+++ gcc/config/mips/mips.md	2007-08-07 10:28:12.000000000 +0100
@@ -360,9 +360,9 @@ (define_attr "length" ""
 	  (eq_attr "type" "const")
 	  (symbol_ref "mips_const_insns (operands[1]) * 4")
 	  (eq_attr "type" "load,fpload")
-	  (symbol_ref "mips_fetch_insns (operands[1]) * 4")
+	  (symbol_ref "mips_load_store_insns (operands[1], insn) * 4")
 	  (eq_attr "type" "store,fpstore")
-	  (symbol_ref "mips_fetch_insns (operands[0]) * 4")
+	  (symbol_ref "mips_load_store_insns (operands[0], insn) * 4")
 
 	  ;; In the worst case, a call macro will take 8 instructions:
 	  ;;
Index: gcc/config/mips/constraints.md
===================================================================
--- gcc/config/mips/constraints.md	2007-08-07 09:52:55.000000000 +0100
+++ gcc/config/mips/constraints.md	2007-08-07 09:53:14.000000000 +0100
@@ -151,7 +151,7 @@ (define_constraint "Q"
 (define_memory_constraint "R"
   "An address that can be used in a non-macro load or store."
   (and (match_code "mem")
-       (match_test "mips_fetch_insns (op) == 1")))
+       (match_test "mips_address_insns (XEXP (op, 0), mode, false) == 1")))
 
 (define_constraint "S"
   "@internal