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.
