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

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

[Nick Clifton]

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

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.

  What do you think, should I apply the patch ?

Cheers
  Nick

gcc/ChangeLog
2007-08-06  Nick Clifton  <nickc@redhat.com>

	* config/mips/mips.c (mips_address_insns): DFmode loads and
	stores only take one instruction when hardware doubles are
	supported.

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;


[-- Attachment #2: 128195.c --]
[-- Type: application/octet-stream, Size: 276 bytes --]

#include <stdio.h>

extern void func (int, int, int);

int
main (void)
{
  static long     long1;
  long            long3;
  static long     long4;
  static double * pd2;
  static double * pd4;

  func (1, (((! *pd2) >= *pd4) * ((long3 * long1) && long4)), 0);

  return 0;
}

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

[Richard Sandiford]

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

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

[Nick Clifton]

Hi Richard,

> 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.

Darn, I was afraid that there might be a case like that.

> 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?

It does.  I also tested the patch on the slightly older sources that make up 
the toolchain that was sent to our customer (which compiles the test case with 
only one ldc1 instruction between a mfhi and a mult instruction) and it fixes 
that toolchain as well.

Cheers
   Nick

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

[Nick Clifton]

Hi Richard,

> 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?

Yes.

> 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.

Are you going to apply this version of the patch, or may I do it on your behalf ?

Cheers
   Nick

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

[Richard Sandiford]

Nick Clifton <nickc@redhat.com> writes:
> Hi 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.
>
> Are you going to apply this version of the patch, or may I do it on
> your behalf ?

Sorry for the delay.  I've been really busy recently, and only started
running tests for the patch yesterday.  I'll commit it today or tomorrow
if the results are OK.

Richard

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

[Richard Sandiford]

Richard Sandiford <richard@codesourcery.com> writes:
> Nick Clifton <nickc@redhat.com> writes:
>> Hi 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.
>>
>> Are you going to apply this version of the patch, or may I do it on
>> your behalf ?
>
> Sorry for the delay.  I've been really busy recently, and only started
> running tests for the patch yesterday.  I'll commit it today or tomorrow
> if the results are OK.

Now committed after testing on mipsisa32r2-elfoabi.

Richard