Re: PING^2: resubmitted IRA improvement patches

[Vladimir Makarov <vmakarov at redhat dot com>]

 On 10/22/2010 01:06 PM, Kenneth Zadeck wrote:
> Dear Vlad,
>
> I have now read patches 2-4 several times and at this point I think that
> the patches fall far below the level of documentation that we need to
> see for IRA.   This is not a matter of getting the english and grammar
> correct.  That can be done later.   What is missing is any high level
> documentation as to the new data structures that have been introduced
> should work as well as a description of how the new algorithms work.
Kenny, thanks for your work.

I updated high-level description of IRA (for changes I've made) which is 
on the top of ira.c files.  This description is already almost 5 pages.

But probably I could improve it more.  The documentation is always not 
enough.
> I use as an example the function improve_allocation at line 3516 of
> patch 2.  This function is about 180 lines of code with no interior
> comments.  The only comment is the mandatory block comment at the top
> which is content free.
>
>
> /* Improve the allocation by spilling some allocnos and assigning the
>     freed hard registers to other allocnos if it decreases the overall
>     allocation cost.  */
>
> This comment conveys no useful information about what this function does
> beyond what could be inferred from the name.
> What is important to know is which allocnos are going to be spilled and
> why those have been chosen over other allocnos.   This is at the heart
> of the allocation, and there is no useful documentation.  If this was
> just isolated to this function, then that would be fine and easily
> handled.   But there is almost nothing in these patches at all that
> describes a high level purpose or view.   And virtually all of the block
> comments at the start of the function are this meaningless.
I thought the function has still pretty forward algorithm which is 
easily understandable from the code.
But what looks obvious for me (or other people who looked at the patch 
before) can be not obvious for other people.  I'll fix it.

It would be useful if you point me out other such places.
> There are two purposes of a review, one is to make sure that the patches
> are mechanically correct, i.e. that there is a comment before each
> function and that the indenting and spelling are done according to gcc
> requirements.    However, the second, and more important is that the
> reviewer be able to understand the algorithms and data structures that
> are being added to, changed or removed.    With respect to this, these
> patches fall far short.
>
> For many passes in GCC, it is sufficient to say that this patch is an
> implementation of some standard published technique.   However, as the
> ira evolves away from the published technique, the documentation needs
> to improve also.   I am not criticizing the decision that have been made
> as you enhance ira and evolve further away from Briggs.   All that i am
> saying is that it has gotten almost impossible to see how all of this
> hangs together.
As I wrote already I updated high-level description of IRA to reflect 
changes in the patches.  This description is almost 5 pages long.  
Probably it is not enough.

Although I wrote 2 articles about IRA because Chaitin-Briggs algorithm 
are probably 1/4 part of IRA.

I feel now that I should write an article about removing classes in IRA 
because there are no substitute for good article to understand a new 
original stuff.  I planned to do it later but probably I should start 
sooner now.

I also going to put high-level description of big Bernd's work (that is 
what worries me more because it is not reflected at all in the 
high-level description).

> For these patches to be accepted, there has to be some meaningful
> comments for the core functions inside of ira.
That is ok.   Now I am planning to include this stuff for gcc4.7.  
Actually, I am going to work on IRA without cover classes more.  This is 
just an intermediate stage which I thought could be helpful.

Too bad that the biggest target winner is PPC (with almost 2% 
performance improvement on SPECFP200).  But probably you are right a 
good documentation is more important for IRA long term future.