Re: RFA: another patch to solve PR49154
On Tue, 31 May 2011, Richard Sandiford wrote:
Gah, seems like I'd forgotten the no subclasses bit by the time
I started looking at code. Sorry for the false alarm.
Still, the extra look made me realise that I should have
restricted that statement to allocatable registers.
(And I really do appreciate a look from a native speaker.)
Updated patch follows, checked dvi and info output:
* doc/tm.texi.in (Register Classes): Document rule for the narrowest
register classes.
* doc/tm.texi: Regenerate.
Index: doc/tm.texi.in
===
--- doc/tm.texi.in (revision 174376)
+++ doc/tm.texi.in (working copy)
@@ -2327,6 +2327,12 @@ constraints is through machine-dependent
You can define such letters to correspond to various classes, then use
them in operand constraints.
+You must define the narrowest register classes for allocatable
+registers, so that each class either has no subclasses, or that for
+some mode, the move cost between registers within the class is
+cheaper than moving a register in the class to or from memory
+(@pxref{Costs}).
+
You should define a class for the union of two classes whenever some
instruction allows both classes. For example, if an instruction allows
either a floating point (coprocessor) register or a general register for a
brgds, H-P
Re: RFA: another patch to solve PR49154
Hans-Peter Nilsson h...@bitrange.com writes:
Index: tm.texi.in
===
--- tm.texi.in(revision 174376)
+++ tm.texi.in(working copy)
@@ -2327,6 +2327,11 @@ constraints is through machine-dependent
You can define such letters to correspond to various classes, then use
them in operand constraints.
+You must define the narrowest register class for a register so that
+class either has no subclasses, or that for some mode, the move cost
+between registers within the class are cheaper than moving a register
+in the class to or from memory (@pxref{Costs}).
+
I fear this isn't true for some MIPS classes.
Richard
Re: RFA: another patch to solve PR49154
On Tue, 31 May 2011, Richard Sandiford wrote:
Hans-Peter Nilsson h...@bitrange.com writes:
Index: tm.texi.in
===
--- tm.texi.in (revision 174376)
+++ tm.texi.in (working copy)
@@ -2327,6 +2327,11 @@ constraints is through machine-dependent
You can define such letters to correspond to various classes, then use
them in operand constraints.
+You must define the narrowest register class for a register so that
+class either has no subclasses, or that for some mode, the move cost
+between registers within the class are cheaper than moving a register
+in the class to or from memory (@pxref{Costs}).
+
I fear this isn't true for some MIPS classes.
I fear the assert will strike then, when there are allocatable
registers in such a class. :)
You don't happen to have target and options to cc1?
brgds, H-P
Re: RFA: another patch to solve PR49154
On Thu, 26 May 2011, Hans-Peter Nilsson wrote: Good, a way forward (well, for me) even though the cure is suspiciously incidental. Thanks for your patience. JFTR, setting CRIS_CC0_REGNUM to fixed just moves the assert to trig at the same place in the v10 multilib, where CRIS_MOF_REGNUM is available and allocatable. With regards to regclasses it looks just like CRIS_CC0_REGNUM, so no big surprise. I'll have to debug this myself and throw a patch at you - or suggest documentation updates to rationalise the change to add a separate subclass for CRIS_SRP_REGNUM! brgds, H-P
Re: RFA: another patch to solve PR49154
On Thu, 26 May 2011, Vladimir Makarov wrote:
On 05/26/2011 04:47 AM, Hans-Peter Nilsson wrote:
On Wed, 25 May 2011, Vladimir Makarov wrote:
It sounds like you're saying that the narrowest register
classes must be formed of registers for which there are trivial
instructions to move between registers inside the class?
No it is wrong. For example, SPARC FPCC (floating point control code
registers) should form a uniform class but there are no trvial insns to move
between registers inside the class.
Yah, the code was tweaked to explicitly handle that case by
excluding that condition for regclasses without subclasses.
Is the following update ok with you (and doc maintainers)?
It says what the code requires, and seems a simple enough rule.
It also fills a documentation gap at the narrow end of the
spectrum of register classes. (I'll update the CRIS port to fit.)
If not, I can tweak the code instead (likely to also exclude the
test for the narrowest code class to which a register belongs),
and regtest that on the compilefarm machines, which IIRC should
cover all the arch's you tested. But that'd require the code to
keep handling non-minimal regclasses with side-conditions on
moves as pressure classes, which seems less desirable to me, and
judging from your willingness to patch a target the other way, I
believe you agree. ;-)
Tested by generating and inspecting dvi and info.
gcc:
* doc/tm.texi.in (Register Classes): Document rule for the narrowest
register classes.
* doc/tm.texi: Regenerate.
Index: tm.texi.in
===
--- tm.texi.in (revision 174376)
+++ tm.texi.in (working copy)
@@ -2327,6 +2327,11 @@ constraints is through machine-dependent
You can define such letters to correspond to various classes, then use
them in operand constraints.
+You must define the narrowest register class for a register so that
+class either has no subclasses, or that for some mode, the move cost
+between registers within the class are cheaper than moving a register
+in the class to or from memory (@pxref{Costs}).
+
You should define a class for the union of two classes whenever some
instruction allows both classes. For example, if an instruction allows
either a floating point (coprocessor) register or a general register for a
brgds, H-P
Re: RFA: another patch to solve PR49154
On Sat, 28 May 2011, Hans-Peter Nilsson wrote: +You must define the narrowest register class for a register so that +class either has no subclasses, or that for some mode, the move cost +between registers within the class are cheaper than moving a register Bah: is cheaper is the grammatically correct version. (N.B. there is only one cost per class.) brgds, H-P
Re: RFA: another patch to solve PR49154
On Wed, 25 May 2011, Vladimir Makarov wrote: On 11-05-25 6:58 PM, Hans-Peter Nilsson wrote: On Wed, 25 May 2011, Vladimir Makarov wrote: This patch solves http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154 for CRIS. The problem was in that the pressure classes did not contain SRP register and the assert failed. I'm not sure I understand the basic requirement. It is ambiguous area. Register pressure classes are used for register pressure evaluation in several parts of the compiler: register pressure sensitive loop invariant motion register pressure sensitive insn scheduling and in IRA to form regions for allocation In some way register pressure classes are what cover classes were when they were defined. Just a note here; for cover classes, SPECIAL_REGS was sufficient. But right definition of register pressure classes are not so critical as the right definition of cover classes were earlier because register pressure calculation is in some way approximation (even if we know what exact classes will be used for each pseudo during the allocation, but we don't know it yet exactly when the register pressure is calculated) because classes used for allocation (allocno classes) are calculated dynamicly and they can be different from register pressure classes and the old cover classes and, the most important thing, the allocno classes can be intersected in any way which makes the register pressure caclulation is inaccurate. Still the register pressure calculation is useful. I added an assertion which checks that the calculated register pressure classes contains all allocatable hard registers. When the assertion fails we have this problem. But the compiler will work well even if the assertion fails. Generally speaking, we could remove the assertion without harm. The assertion is just a check for *the most* targets that the pressure classes calculation is not broken because for the most targets the register pressure classes should contain all available hard registers. The assertion failed for CRIS because we had pressure classes only CC0_REGS and GENERAL_REGS which do not includes SRP register. It sounds like a bug that CC0_REGS was considered at all, as the only register is not allocatable. SPECIAL_REGS should be chosen; the only allocatable register would be SRP_REGS (additionally, MOF_REGS for the v10 multilib). I'll have a look. Can you please suggest an update to the target macro documentation to reflect the requirement it's currently failing? To feel ok with this change I need to understand why it's not ok as is: I can't see the error, just a random change narrowing a register class that at a glance *should* not be needed. To wit, I need to understand why the bug is here and that it's not the failing assert in IRA that's wrong. I don't think it is necessary because as I know only CRIS requires register classes modification. On the contrary. I think at least a should needs to change to must regarding register classes, or we can't say what to change. The documentation is not what works for most targets! It sounds like you're saying that the narrowest register classes must be formed of registers for which there are trivial instructions to move between registers inside the class? I think that'd be reasonable and if you agree I'll do the update. In either case, your patch wouldn't be complete as more changes are needed, for example for secondary reloads the new SRP_REGS has to be considered. brgds, H-P
Re: RFA: another patch to solve PR49154
On 05/26/2011 04:47 AM, Hans-Peter Nilsson wrote: On Wed, 25 May 2011, Vladimir Makarov wrote: On 11-05-25 6:58 PM, Hans-Peter Nilsson wrote: On Wed, 25 May 2011, Vladimir Makarov wrote: This patch solves http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154 for CRIS. The problem was in that the pressure classes did not contain SRP register and the assert failed. I'm not sure I understand the basic requirement. It is ambiguous area. Register pressure classes are used for register pressure evaluation in several parts of the compiler: register pressure sensitive loop invariant motion register pressure sensitive insn scheduling and in IRA to form regions for allocation In some way register pressure classes are what cover classes were when they were defined. Just a note here; for cover classes, SPECIAL_REGS was sufficient. But right definition of register pressure classes are not so critical as the right definition of cover classes were earlier because register pressure calculation is in some way approximation (even if we know what exact classes will be used for each pseudo during the allocation, but we don't know it yet exactly when the register pressure is calculated) because classes used for allocation (allocno classes) are calculated dynamicly and they can be different from register pressure classes and the old cover classes and, the most important thing, the allocno classes can be intersected in any way which makes the register pressure caclulation is inaccurate. Still the register pressure calculation is useful. I added an assertion which checks that the calculated register pressure classes contains all allocatable hard registers. When the assertion fails we have this problem. But the compiler will work well even if the assertion fails. Generally speaking, we could remove the assertion without harm. The assertion is just a check for *the most* targets that the pressure classes calculation is not broken because for the most targets the register pressure classes should contain all available hard registers. The assertion failed for CRIS because we had pressure classes only CC0_REGS and GENERAL_REGS which do not includes SRP register. It sounds like a bug that CC0_REGS was considered at all, as the only register is not allocatable. SPECIAL_REGS should be chosen; the only allocatable register would be SRP_REGS (additionally, MOF_REGS for the v10 multilib). I'll have a look. Can you please suggest an update to the target macro documentation to reflect the requirement it's currently failing? To feel ok with this change I need to understand why it's not ok as is: I can't see the error, just a random change narrowing a register class that at a glance *should* not be needed. To wit, I need to understand why the bug is here and that it's not the failing assert in IRA that's wrong. I don't think it is necessary because as I know only CRIS requires register classes modification. On the contrary. I think at least a should needs to change to must regarding register classes, or we can't say what to change. The documentation is not what works for most targets! It sounds like you're saying that the narrowest register classes must be formed of registers for which there are trivial instructions to move between registers inside the class? No it is wrong. For example, SPARC FPCC (floating point control code registers) should form a uniform class but there are no trvial insns to move between registers inside the class. I'll think how to formulate the requirement but it will be really not easy. I think that'd be reasonable and if you agree I'll do the update. In either case, your patch wouldn't be complete as more changes are needed, for example for secondary reloads the new SRP_REGS has to be considered. I've checked CC0 and it is not fixed. If I make it fixed, I have pressure classes SPECIAL_REGS and GENERAL_REGS and the assertion is ok. But you need another patch for PR49154, the one I sent to fix SPARC.
Re: RFA: another patch to solve PR49154
On Thu, 26 May 2011, Vladimir Makarov wrote: On 05/26/2011 04:47 AM, Hans-Peter Nilsson wrote: On Wed, 25 May 2011, Vladimir Makarov wrote: It sounds like you're saying that the narrowest register classes must be formed of registers for which there are trivial instructions to move between registers inside the class? No it is wrong. For example, SPARC FPCC (floating point control code registers) should form a uniform class but there are no trvial insns to move between registers inside the class. I see. I'll think how to formulate the requirement but it will be really not easy. I'd be very thankful but moreover we don't have to inspect the source and guess and maybe ultimately ask you why the abort triggered every time there's a new port or someone changes the register classes in their port. :) I think that'd be reasonable and if you agree I'll do the update. In either case, your patch wouldn't be complete as more changes are needed, for example for secondary reloads the new SRP_REGS has to be considered. I've checked CC0 and it is not fixed. If I make it fixed, I have pressure classes SPECIAL_REGS and GENERAL_REGS and the assertion is ok. But you need another patch for PR49154, the one I sent to fix SPARC. Oops, it should be, at least until the CRIS port is de-CC0:ed. I'll fix that. Still, I think I'll have a look why SRP was forgotten. Good, a way forward (well, for me) even though the cure is suspiciously incidental. Thanks for your patience. brgds, H-P
RFA: another patch to solve PR49154
This patch solves http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154 for
CRIS. The problem was in that the pressure classes did not contain SRP
register and the assert failed.
OK to commit?
2011-05-25 Vladimir Makarov vmaka...@redhat.com
PR rtl-optimization/49154
* config/cris/cris.h (reg_class, REG_CLASS_NAMES)
(REG_CLASS_CONTENTS, REGNO_REG_CLASS): Add class SRP_REGS.
Index: config/cris/cris.h
===
--- config/cris/cris.h (revision 174219)
+++ config/cris/cris.h (working copy)
@@ -498,7 +498,7 @@ extern int cris_cpu_version;
enum reg_class
{
NO_REGS,
-ACR_REGS, MOF_REGS, CC0_REGS, SPECIAL_REGS,
+ACR_REGS, SRP_REGS, MOF_REGS, CC0_REGS, SPECIAL_REGS,
SPEC_ACR_REGS, GENNONACR_REGS,
SPEC_GENNONACR_REGS, GENERAL_REGS,
ALL_REGS,
@@ -507,10 +507,10 @@ enum reg_class
#define N_REG_CLASSES (int) LIM_REG_CLASSES
-#define REG_CLASS_NAMES\
- {NO_REGS, \
- ACR_REGS, MOF_REGS, CC0_REGS, SPECIAL_REGS, \
- SPEC_ACR_REGS, GENNONACR_REGS, SPEC_GENNONACR_REGS, \
+#define REG_CLASS_NAMES
\
+ {NO_REGS, \
+ ACR_REGS, SRP_REGS, MOF_REGS, CC0_REGS, SPECIAL_REGS, \
+ SPEC_ACR_REGS, GENNONACR_REGS, SPEC_GENNONACR_REGS, \
GENERAL_REGS, ALL_REGS}
#define CRIS_SPECIAL_REGS_CONTENTS \
@@ -521,6 +521,7 @@ enum reg_class
{\
{0},\
{1 CRIS_ACR_REGNUM}, \
+ {1 CRIS_SRP_REGNUM}, \
{1 CRIS_MOF_REGNUM}, \
{1 CRIS_CC0_REGNUM}, \
{CRIS_SPECIAL_REGS_CONTENTS}, \
@@ -540,7 +541,7 @@ enum reg_class
((REGNO) == CRIS_ACR_REGNUM ? ACR_REGS : \
(REGNO) == CRIS_MOF_REGNUM ? MOF_REGS : \
(REGNO) == CRIS_CC0_REGNUM ? CC0_REGS : \
- (REGNO) == CRIS_SRP_REGNUM ? SPECIAL_REGS : \
+ (REGNO) == CRIS_SRP_REGNUM ? SRP_REGS : \
GENERAL_REGS)
#define BASE_REG_CLASS GENERAL_REGS
Re: RFA: another patch to solve PR49154
On 11-05-25 6:58 PM, Hans-Peter Nilsson wrote: On Wed, 25 May 2011, Vladimir Makarov wrote: This patch solves http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49154 for CRIS. The problem was in that the pressure classes did not contain SRP register and the assert failed. I'm not sure I understand the basic requirement. It is ambiguous area. Register pressure classes are used for register pressure evaluation in several parts of the compiler: register pressure sensitive loop invariant motion register pressure sensitive insn scheduling and in IRA to form regions for allocation In some way register pressure classes are what cover classes were when they were defined. But right definition of register pressure classes are not so critical as the right definition of cover classes were earlier because register pressure calculation is in some way approximation (even if we know what exact classes will be used for each pseudo during the allocation, but we don't know it yet exactly when the register pressure is calculated) because classes used for allocation (allocno classes) are calculated dynamicly and they can be different from register pressure classes and the old cover classes and, the most important thing, the allocno classes can be intersected in any way which makes the register pressure caclulation is inaccurate. Still the register pressure calculation is useful. I added an assertion which checks that the calculated register pressure classes contains all allocatable hard registers. When the assertion fails we have this problem. But the compiler will work well even if the assertion fails. Generally speaking, we could remove the assertion without harm. The assertion is just a check for *the most* targets that the pressure classes calculation is not broken because for the most targets the register pressure classes should contain all available hard registers. The assertion failed for CRIS because we had pressure classes only CC0_REGS and GENERAL_REGS which do not includes SRP register. We cannot use SPECIAL_REGS for pressure classes because it contains too different registers (the algorithm finding register pressure classes looks how moves in the class is costly, e.g. moves from CC0_REGS and SRP which are part of SPECIAL_REGS is costly). Adding class SRP_REGS, we forms pressure classes SRP_REGS, CC0_REGS, and GENERAL_REGS which contain all available hard regs and the assertion becomes true. So I could remove the assertion or add SRP_REGS class for CRIS. I prefer the 2nd. Coloring will not change after adding SRP_REGS because we use dinamicaly calculated classes (simply hard reg sets which are subsets of the allocno classes) which can be different from classes defined by the target dependent files. I hope that the details I wrote here will help to understand the reasons for the problem but as I wrote it is very ambiguous area. OK to commit? 2011-05-25 Vladimir Makarovvmaka...@redhat.com PR rtl-optimization/49154 * config/cris/cris.h (reg_class, REG_CLASS_NAMES) (REG_CLASS_CONTENTS, REGNO_REG_CLASS): Add class SRP_REGS. Can you please suggest an update to the target macro documentation to reflect the requirement it's currently failing? To feel ok with this change I need to understand why it's not ok as is: I can't see the error, just a random change narrowing a register class that at a glance *should* not be needed. To wit, I need to understand why the bug is here and that it's not the failing assert in IRA that's wrong. I don't think it is necessary because as I know only CRIS requires register classes modification. But I'd say the general rule would be defining more classes as possible (each class for uniform groups of registers and for different unions of them). It would help to avoid such problem and improve coloring. On the other hand adding all the different unions would be probably overkill for the most targets. (And thanks for your work, really. Don't worry about fitting the update into texinfo format, I can help with that.)
