Re: IRA reload review.

[Bernd Schmidt <bernds_cb1 at t-online dot de>]

Vladimir Makarov wrote:
> After you asked me split them up, I sent the parts to make easy to 
> review the reload changes.  They were not real patches because they were 
> extracted from IRA patch.  That was probably my mistake, I should have 
> sent the real patches.
>
> http://gcc.gnu.org/ml/gcc-patches/2008-06/msg00535.html
>
> You wrote that you need real patches and I promised to do this and 
> started to work on them

It's not exactly what I asked for.  I had hoped that IRA could be merged 
not as a monolithic block, but in multiple stages.  As I've been trying 
to say, committing smaller, independent patches leads to better review, 
easier debugging when problems show up, and a better understanding of 
the benefits of the individual patches (via benchmarking).  IMO this 
would have produced a better end result.

The first split-up attempt was good to see which more-or-less 
independent pieces exist.  But several of these had descriptions which 
did not indicate why the improvement could only be used in conjunction 
with IRA, or why IRA would rely on them to work.  If neither is the case 
for any given patch, then IMO it should have been applied and 
performance tested separately.  For example, let's pick one of them at 
random:

"The patch implements finding reloads for most frequently executed
insns first.  Sorting insn chain is done for this by calling
sort_insn_chain in reload1.c::reload.  By processing most frequently
executed insns first, we have a chance to provide them better hard
registers for the reloads."

While it modifies ira.c, there is at first glance little to suggest that 
this could not have been first implemented for the old register 
allocator.  That would have given us a clue as to what kind of benefit 
we gain from it.

The alternative approach (attacking the problem from the other direction 
so to speak), would be to create a minimal IRA core patch, benchmark it 
on its own against the old register allocator, and then apply 
incremental improvements on top of it.  Your split-up patches would 
probably have been useful for this, but AFAICT there was no such minimal 
core patch posted.

With a monolithic commit we now have a large set of changes, and no good 
idea of how they perform individually, and no easy way to bisect them in 
case of problems.

> What I did not do was benchmarking all changes as you asked.  
> Unfortunatly I had no time to do this because it would have taken a lot 
> of time (probably 1-2 week) and I was and am still busy fixing IRA 
> bugs.

Well, maybe we need to get away from letting development goals be 
decreed by management ("IRA for 4.4") and back to waiting until things 
are ready.

> But I did all these changes to improve the code (mostly SPEC) for 
> my 2 years work on IRA project if there were a degradation I would not 
> put the change on the branch.  So all this changes improves the code in 
> some way.

I fully trust you when you say that, but IMO it would have been 
important to document these things more comprehensively for a massive 
change like this.  Apparently you decided not to merge one of these 
changes just a couple of days ago as it was nonfunctional and had no 
demonstrable benefit.  It would have been good to demonstrate that the 
same is not true for the other changes that were merged.

I would also like to apologize for not being able to commit more time to 
reviewing things.


Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif