* RFA/RFC: GCC has an incorrect length for the ldc1 instruction
@ 2007-08-07 8:07 Nick Clifton
2007-08-07 9:54 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Nick Clifton @ 2007-08-07 8:07 UTC (permalink / raw)
To: echristo, richard; +Cc: gcc-patches
[-- 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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction 2007-08-07 8:07 RFA/RFC: GCC has an incorrect length for the ldc1 instruction Nick Clifton @ 2007-08-07 9:54 ` Richard Sandiford 2007-08-07 15:03 ` Nick Clifton 2007-08-10 7:15 ` Nick Clifton 0 siblings, 2 replies; 6+ messages in thread From: Richard Sandiford @ 2007-08-07 9:54 UTC (permalink / raw) To: Nick Clifton; +Cc: echristo, gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction 2007-08-07 9:54 ` Richard Sandiford @ 2007-08-07 15:03 ` Nick Clifton 2007-08-10 7:15 ` Nick Clifton 1 sibling, 0 replies; 6+ messages in thread From: Nick Clifton @ 2007-08-07 15:03 UTC (permalink / raw) To: Nick Clifton, echristo, gcc-patches, richard 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction 2007-08-07 9:54 ` Richard Sandiford 2007-08-07 15:03 ` Nick Clifton @ 2007-08-10 7:15 ` Nick Clifton 2007-08-10 7:58 ` Richard Sandiford 1 sibling, 1 reply; 6+ messages in thread From: Nick Clifton @ 2007-08-10 7:15 UTC (permalink / raw) To: Nick Clifton, echristo, gcc-patches, richard 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction 2007-08-10 7:15 ` Nick Clifton @ 2007-08-10 7:58 ` Richard Sandiford 2007-08-10 15:41 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: Richard Sandiford @ 2007-08-10 7:58 UTC (permalink / raw) To: Nick Clifton; +Cc: echristo, gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA/RFC: GCC has an incorrect length for the ldc1 instruction 2007-08-10 7:58 ` Richard Sandiford @ 2007-08-10 15:41 ` Richard Sandiford 0 siblings, 0 replies; 6+ messages in thread From: Richard Sandiford @ 2007-08-10 15:41 UTC (permalink / raw) To: Nick Clifton; +Cc: echristo, gcc-patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-08-10 15:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-08-07 8:07 RFA/RFC: GCC has an incorrect length for the ldc1 instruction Nick Clifton 2007-08-07 9:54 ` Richard Sandiford 2007-08-07 15:03 ` Nick Clifton 2007-08-10 7:15 ` Nick Clifton 2007-08-10 7:58 ` Richard Sandiford 2007-08-10 15:41 ` Richard Sandiford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).