This is the mail archive of the gcc-patches@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]

Re: PING^2: resubmitted IRA improvement patches


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.  

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. 

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.   

For these patches to be accepted, there has to be some meaningful
comments for the core functions inside of ira. 

Kenny







On 09/30/2010 04:51 PM, Vladimir Makarov wrote:
>
>
> I can not get an approval of the following patches since middle of
> August.
>
> Jeff Law started a review of the 1st version of these patches in July
> (before their rework to resolve conflicts with big Bernd's patch) but
> he can not continue this work for some personal reasons.
>
> Although it would be more helpful for me to get a maintainer status
> for my code to avoid such situations in the future.
>
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01040.html
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01041.html
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01042.html
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01043.html
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]