Re: [PATCH] rtlopt branch merge part 5 -- loop unswitching

[kenner at vlsi1 dot ultra dot nyu dot edu (Richard Kenner)]

    satisfied?

Better.  I have a few suggestions, though:

    /* Perform this transformation:

I'd start by explaining in words what's going on and then give this as
an example.  So:

/* This pass looks for loops that contain invariant conditions and bring
   those conditions outside of the loop ("swap" the loop and the conditions).
   For example, we transform:

      this might lead to exponential code growth, so we limit the number of
      unswitchings done in a single loop to PARAM_MAX_UNSWITCH_LEVEL.  */

What you say is only true if the condition has an "else", otherwise it
doesn't grow the code.  You should discuss that issue here.

    /* Unswitch LOOPS.  */
    void
    unswitch_loops (loops)

Our coding style requires you say what the parameter means even if
it's "obvious".  You have this problem in most functions.

      /* Removed loop?  */
      loop = loops->parray[i];
      if (!loop)
	continue;
      /* Not innermost?  */
      if (loop->inner)
	continue;

These are like the classic "Add 1 to I" assembler comments in that they
simply repeat the code in English.  In such cases, the comments actually
clutter the code.  So they should either be omitted or expanded on.  Also,
the two simple conditions followed by the same action ("continue") I think
would read easier if merged.  So I'd write this as:

      loop = loops->parray[i];

      /* We only want to perform this optimization on innermost loops
         because that's where the effect will be the greatest.  Also ignore
         loops that have already been removed.  */
      if (!loop || loop->inner)
	continue;

    /* Checks whether we can unswitch LOOP on basic block BB.  LOOP BODY
       is provided to save time.  */

What does "on" mean here?  I think what you mean is that BB is one of the
basic blocks within the loop that's preceded by a test, but it would be good
to clarify exactly where that block is.

    /* Reverses CONDition; returns NULL if we cannot.  */
    static rtx
    reversed_condition (cond)

Don't we have this function in the compiler already?

I didn't look at the rest closely, but I hope you get my point: the key is
not to document what the code is *doing*, since a reader can figure that
out, usually quickly, but to document *why* it's doing what it's doing
and to give as much information as possible as to what each function
is doing, so a reader doesn't have to read the code of the function in
order to figure it out.