Re: An issue for the SC: horrible documentation quality of GCC

[kenner@vlsi1.ultra.nyu.edu (Richard Kenner)]

    I don't know about you.  But when I look at gcse.c, I see heaps of
    comments, before most of the functions.  

The problem is that it's a requirement that it be before *all* functions,
not "most".  I count nine that are missing it and didn't look at how many
are incomplete (see one example below).

    Also before the *local_cprop* functions, except local_cprop_pass(), which
    is only called at toplevel from one_cprop_pass().

But if I'm looking to see what a specific local_cprop function does,
I'm going to look in front of local_cprop_pass, not try to find the function
that calls it and look there.

Now let's consider the documentation of one_cprop_pass.  It refers to a
"copy/constant propagation pass".   I did a search of the entire file for
the string "copy/constant propagation" and find *no* description of what
that term means.  Yes, most people would know what "constant propagation"
is, but what does "copy" mean in that context?

    Also the whole gcse.c is documented on the top of that file rather
    extensively.

No, I don't consider it "extensive" at all.  Yes, it gives a very broad
overview of what's being done, but doesn't link that overview to the
implementation.

    And additionally a heap of paper references are given
    for people really interested in the theoretic background.

That's good, but I suspect that the comments also assume that every
reader has read all the papers.  If it's the desire to rely on a paper
as a prerequisite for documentation, there should be a pointer to *one*
paper and a URL where it can be found.

    Well, on the top of gcse.c it says, that one of the passes in GCSE is
    copy/constant propagation.  This is a standard optimization, and hence
    the actual description of any algorithm (on the high level) seems
    superflous.

I disagree.  There are lots of "standard" optimizations, but once you
go from the realm of a paper into an actual implementation, you need to
say *precisely* what you've done.

    Tricks used in the implementation should of course be
    documented, but have no place in any high level overview comments.

Agreed, but what *is* important in that high-level overview, and is missing,
is an explanation of how the optimizations in this file interact with other
similar optimizations.  Specifically, it should say what the boundaries
between what's done there and what's done in CSE are.

I also see absolutely no discussion of the critical issue of when constants
should replace registers: CSE has a lot of code to do that and if it's being
done different in gcse there needs to be documentation saying why.

    About the actual problem you have: There is code to adjust the
    REG_EQUAL notes for changed libcall blocks, namely in
    adjust_libcall_notes() called from do_local_cprop(), which sounds as
    if it should do what you wanted.  Ergo you probably want to look at
    that function, why it doesn't work.  The third match for "LIBCALL" set
    me on the right track, so it wasn't exactly difficult to find.

OK, but just as an example for what's missing, I'm looking at 

/* LIBCALL_SP is a zero-terminated array of insns at the end of a libcall;
   their REG_EQUAL notes need updating.  */

static bool
do_local_cprop (x, insn, alter_jumps, libcall_sp)

It's nice that *one* operand to that function is documented, but it would
be even better if *all* of them were and better still if there were at least
one sentence saying what the function as a whole is supposed to do.

Getting back to adjust_libcall_notes: there are absolutely no comments
within the routine.  There's a call to reg_set_between_p whose purpose
isn't obvious to me and nothing in the function header comments say why
register usage would be important.  Since there are no comments in the
function itself, there's no way to know what was intended there.  Perhaps
that's related to the bug, perhaps not, but without knowing the *purpose*
of that call, it's hard to say.