This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
| Other format: | [Raw text] | |
Sorry in advance for the long message. And I'm sure this sort of thing
has been discussed before. But anyway...
Reload seems a bit too eager to force constants into memory, at least on
some architectures. And while looking into that, I wondered whether it
was doing enough to cater for SECONDARY_MEMORY_NEEDED.
Say we have:
(set (reg:SI r1) (reg:SI r2))
and:
r1 & r2 are pseudos,
r1 is allocated a general register,
r2 isn't, but is found to be equivalent to a constant C,
C isn't accepted by the movsi constraints
If there are two alternatives:
reg <- reg
reg <- mem
then both are treated as needing one reload (cost: 6 units).
However, this code in find_reloads:
/* We prefer to reload pseudos over reloading other things,
since such reloads may be able to be eliminated later.
If we are reloading a SCRATCH, we won't be generating any
insns, just using a register, so it is also preferred.
So bump REJECT in other cases. Don't do this in the
case where we are forcing a constant into memory and
it will then win since we don't want to have a different
alternative match then. */
if (! (GET_CODE (operand) == REG
&& REGNO (operand) >= FIRST_PSEUDO_REGISTER)
&& GET_CODE (operand) != SCRATCH
&& ! (const_to_mem && constmemok))
reject += 2;
will increase the cost of the reg <- reg alternative by 2 units,
deliberately making the reg <- mem one cheaper.
I can see that that makes sense for targets where symbol_refs are valid
addresses. But when they aren't, we'll end up having to reload the
constant pool address into a register. For example, on mips-linux-gnu,
we'd get the equivalent of:
.LC0: .word C
la temp,.LC0
lw r1,(temp)
See the attached stress.c for a brute-force example.
So, my question is: do we really need to prefer reg <- mem over
reg <- reg on targets that will need to reload the pool address?
Choosing reg <- reg will mean that the input reload will "rematerialise"
the constant (by calling the move expanders or by using the reload_in
pattern). And reload's optimisers are often able to remove any
redundant reg <- reg moves that this might create.
I tried two ways of making the reg <- mem alternative more costly:
- Disable the const_to_mem get-out clause above if symbol_refs aren't
valid addresses. [patch-1.diff]
- (More aggressive:) Count the expected symbol_ref reload as a full
extra reload. [patch-2.diff]
The patches are supposed to be mutually exclusive. They follow the
existing convention of testing for addressing modes in init_reload.
To see how the changes affect ports other than mips, I tried taking one
configuration from each backend[*] and diffing the output of c-torture.
I'd copied stress.c to c-torture first, so it's included in the analysis
below.
([*] aside from n32k & iq2000, which didn't build)
All files were compiled with: <dir>/xgcc -B<dir>/ -O2 -S foo.c -o foo.s
The results were as follows (ignore the last column for now ;):
CVS Patch 1 Patch 2
vs. vs. vs.
Patch 1 Patch 2 Patch 3
alpha-linux-gnu same better?
arc-elf n/a n/a
arm-elf same n/a
avr-elf n/a n/a
c4x-elf n/a n/a
cris-elf n/a n/a
d30v-elf n/a n/a
dsp16xx-elf same n/a
fr30-elf same n/a
frv-elf same n/a
h8300-elf n/a n/a
hppa-linux-gnu same better?
i370-linux-gnu same n/a
i686-linux-gnu n/a better?
ia64-linux-gnu same n/a
ip2k-elf same n/a
m32r-elf same n/a
m68hc11-elf n/a n/a
m68k-elf n/a n/a
mcore-elf ---- n/a
mips-linux-gnu better n/a
mmix n/a n/a
mn10300-elf n/a n/a
pdp11-elf n/a n/a
powerpc-linux-gnu better? same
s390-linux-gnu ---- ---- same
sh64-elf same better? n/a
sparc-linux-gnu better? worse better? (= patch 1)
v850-elf better? n/a
xstormy16-elf same n/a
vax-ultrix n/a n/a
xtensa-elf same n/a
Key to "CVS vs. Patch 1":
n/a: The target allows symbolic addresses, so the patch has no effect.
same: The target doesn't allow symbolic addresses but the patch
has no effect anyway. Presumably the md patterns had matching
constant constraints.
better: The output looks better to me. I'm not real judge of
powerpc, sparc or v850 code though, so I've attached the diffs
below.
There were two problems:
- mcore-elf ICEd for a few more tests. It seems that the movsi
patterns can't handle all legitimate constants when called
during reload.
- There are lots of s390-linux-gnu changesm, some of which
look better and some of which look worse. I've attached
the diff for reference, but...
As far as s390 goes, the init_reload test wasn't really supposed to
fail in the first place. The port _does_ treat constant pool addresses
as legitimate.
The problem is that the test in init_reload is for a general symbol_ref,
not a constant pool address. I couldn't think of a good way of fixing
this within the current framework. For example, if we set the dummy
symbol's CONSTANT_POOL_ADDRESS_P bit, the backend might pass the
symbol to functions like get_pool_mode(), which would then abort.
-> One alternative would be to add a target hook. It could take a
mode as argument and return a bitset of supported addressing types:
reg + reg, constant pool address, indirect memref, etc. Maybe we
could have a default implementation that does something akin to
init_reload.
Anyway, in terms of output code, patch 1 seems to be a strict
improvement for everything except s390. That's including mcore,
since there's an improvement to one test and no change in the others.
But before trying to fix the mcore & s390 problems, I thought I'd see
whether the general idea is OK. What do you think?
Moving on to patch 2... it only differed from patch 1 on two targets:
sh64-elf (for which it seems to be an improvement) and sparc-linux-gnu,
for which it was a regression. Diffs for both attached.
Personally, I slightly prefer patch 2 to patch 1. Giving the symbol_ref
reload a cost of 6 units seems more in keeping with the surrounding code.
So, I tried to track down the source of the sparc problem.
We had insns like:
(set (reg:SF %fx) (const_double:SF 0))
which matched *movsf_insn_novis. The best two alternatives were r <- G
and f <- m, with the following costs:
CVS Patch 1 Patch 2
(1) r <- G 9 9 9
(6) f <- m 6 8 12
The cost of alternative (1) seems a little low to me since the output
reload has to be done through memory. If we count it as two reloads
instead of one, using:
#ifdef SECONDARY_MEMORY_NEEDED
if (GET_CODE (operand) == REG
&& REGNO (operand) < FIRST_PSEUDO_REGISTER
&& this_alternative[i] != NO_REGS
&& (SECONDARY_MEMORY_NEEDED
(this_alternative[i],
REGNO_REG_CLASS (REGNO (operand)),
GET_MODE (operand))))
losers++;
#endif
then the results for patch 2 are the same as for patch 1 on sparc.
I've attached this modified version of patch 2 as patch 3. I tried
running it through all targets that define SECONDARY_MEMORY_NEEDED,
including those that accept symbol_ref addresses. Aside from s390,
it seems to be a strict improvement compared with CVS and patch 2.
Diffs attached once again. ;)
FWIW, patch 2 was bootstrapped & regression tested on various
mips targets. But this isn't really an RFA because of the s390
& mcore issues.
Richard
Attachment:
stress.c
Description: Text document
Index: reload.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.h,v
retrieving revision 1.45
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.45 reload.h
--- reload.h 6 Jul 2003 09:56:07 -0000 1.45
+++ reload.h 24 Aug 2003 19:52:51 -0000
@@ -187,6 +187,8 @@ extern int reload_first_uid;
extern char indirect_symref_ok;
+extern bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
extern char double_reg_address_ok;
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.406
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.406 reload1.c
--- reload1.c 19 Jul 2003 14:47:13 -0000 1.406
+++ reload1.c 24 Aug 2003 19:52:53 -0000
@@ -226,6 +230,9 @@ static char spill_indirect_levels;
which these are valid is the same as spill_indirect_levels, above. */
char indirect_symref_ok;
+/* Nonzero if symbolic addresses are legitimate. */
+bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
char double_reg_address_ok;
@@ -454,6 +461,10 @@ init_reload (void)
tem = gen_rtx_MEM (Pmode, gen_rtx_SYMBOL_REF (Pmode, "foo"));
indirect_symref_ok = memory_address_p (QImode, tem);
+
+ /* See whether (SYMBOL_REF ...) itself is a legitimate address. */
+
+ symbolic_address_ok = memory_address_p (word_mode, XEXP (tem, 0));
/* See if reg+reg is a valid (and offsettable) address. */
Index: reload.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.c,v
retrieving revision 1.220
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.220 reload.c
--- reload.c 19 Jul 2003 14:47:13 -0000 1.220
+++ reload.c 24 Aug 2003 19:52:55 -0000
@@ -3404,7 +3404,7 @@ find_reloads (rtx insn, int replace, int
if (! (GET_CODE (operand) == REG
&& REGNO (operand) >= FIRST_PSEUDO_REGISTER)
&& GET_CODE (operand) != SCRATCH
- && ! (const_to_mem && constmemok))
+ && ! (const_to_mem && constmemok && symbolic_address_ok))
reject += 2;
/* Input reloads can be inherited more often than output
Index: reload.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.h,v
retrieving revision 1.45
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.45 reload.h
--- reload.h 6 Jul 2003 09:56:07 -0000 1.45
+++ reload.h 24 Aug 2003 19:50:24 -0000
@@ -187,6 +187,8 @@ extern int reload_first_uid;
extern char indirect_symref_ok;
+extern bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
extern char double_reg_address_ok;
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.406
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.406 reload1.c
--- reload1.c 19 Jul 2003 14:47:13 -0000 1.406
+++ reload1.c 24 Aug 2003 19:50:26 -0000
@@ -226,6 +230,9 @@ static char spill_indirect_levels;
which these are valid is the same as spill_indirect_levels, above. */
char indirect_symref_ok;
+/* Nonzero if symbolic addresses are legitimate. */
+bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
char double_reg_address_ok;
@@ -454,6 +461,10 @@ init_reload (void)
tem = gen_rtx_MEM (Pmode, gen_rtx_SYMBOL_REF (Pmode, "foo"));
indirect_symref_ok = memory_address_p (QImode, tem);
+
+ /* See whether (SYMBOL_REF ...) itself is a legitimate address. */
+
+ symbolic_address_ok = memory_address_p (word_mode, XEXP (tem, 0));
/* See if reg+reg is a valid (and offsettable) address. */
Index: reload.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.c,v
retrieving revision 1.220
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.220 reload.c
--- reload.c 19 Jul 2003 14:47:13 -0000 1.220
+++ reload.c 24 Aug 2003 19:50:28 -0000
@@ -3366,7 +3366,8 @@ find_reloads (rtx insn, int replace, int
&& operand_mode[i] != VOIDmode)
{
const_to_mem = 1;
- if (this_alternative[i] != (int) NO_REGS)
+ if (this_alternative[i] != (int) NO_REGS
+ || !symbolic_address_ok)
losers++;
}
Index: reload.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.h,v
retrieving revision 1.45
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.45 reload.h
--- reload.h 6 Jul 2003 09:56:07 -0000 1.45
+++ reload.h 24 Aug 2003 19:49:43 -0000
@@ -187,6 +187,8 @@ extern int reload_first_uid;
extern char indirect_symref_ok;
+extern bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
extern char double_reg_address_ok;
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.406
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.406 reload1.c
--- reload1.c 19 Jul 2003 14:47:13 -0000 1.406
+++ reload1.c 24 Aug 2003 19:49:46 -0000
@@ -226,6 +230,9 @@ static char spill_indirect_levels;
which these are valid is the same as spill_indirect_levels, above. */
char indirect_symref_ok;
+/* Nonzero if symbolic addresses are legitimate. */
+bool symbolic_address_ok;
+
/* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid. */
char double_reg_address_ok;
@@ -454,6 +461,10 @@ init_reload (void)
tem = gen_rtx_MEM (Pmode, gen_rtx_SYMBOL_REF (Pmode, "foo"));
indirect_symref_ok = memory_address_p (QImode, tem);
+
+ /* See whether (SYMBOL_REF ...) itself is a legitimate address. */
+
+ symbolic_address_ok = memory_address_p (word_mode, XEXP (tem, 0));
/* See if reg+reg is a valid (and offsettable) address. */
Index: reload.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload.c,v
retrieving revision 1.220
diff -u -d -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.220 reload.c
--- reload.c 19 Jul 2003 14:47:13 -0000 1.220
+++ reload.c 24 Aug 2003 19:49:48 -0000
@@ -3366,7 +3366,8 @@ find_reloads (rtx insn, int replace, int
&& operand_mode[i] != VOIDmode)
{
const_to_mem = 1;
- if (this_alternative[i] != (int) NO_REGS)
+ if (this_alternative[i] != (int) NO_REGS
+ || !symbolic_address_ok)
losers++;
}
@@ -3392,6 +3393,17 @@ find_reloads (rtx insn, int replace, int
else if (modified[i] != RELOAD_WRITE && no_input_reloads
&& ! const_to_mem)
bad = 1;
+
+#ifdef SECONDARY_MEMORY_NEEDED
+ if (GET_CODE (operand) == REG
+ && REGNO (operand) < FIRST_PSEUDO_REGISTER
+ && this_alternative[i] != NO_REGS
+ && (SECONDARY_MEMORY_NEEDED
+ (this_alternative[i],
+ REGNO_REG_CLASS (REGNO (operand)),
+ GET_MODE (operand))))
+ losers++;
+#endif
/* We prefer to reload pseudos over reloading other things,
since such reloads may be able to be eliminated later.
Attachment:
patch-effects.tar.bz2
Description: BZip2 compressed data
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |