Re: [PATCH] Modulo-scheduling improvements. Patch 2 of 2.

[Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>]

Hello,

> See below comments for your patch 2 of 2 (
> http://gcc.gnu.org/ml/gcc-patches/2007-01/msg01470.html).
> 
> Note that you need an spu maintainer to approve the spu.md part, and a loop
> optimizer maintainer to approve the doloop part.

for the doloop part:

> +       * loop-doloop.c: (doloop_optimize) support for the a doloop pattern.
> +       (doloop_condition_get) gets an instruction instead of a pattern.
> +       (doloop_optimize) support for the new doloop pattern.
> +       and new doloop pattern support
> +       (doloop_modify): Update the REG_BR_PROB note.

Wrong formating of the changelog entry.

> /* Return the loop termination condition for PATTERN or zero
> !    if it is not a decrement and branch jump insn or a sequence
> !    of decrement and branch_ne istructions.  */
> 
>   rtx
>   ! doloop_condition_get (rtx doloop_pat)

Fix the comment -- the name of the argument should match, and you should
describe what DOLOOP_PAT is -- since it is an insn and not a
pattern, I would also suggest changing DOLOOP_PAT name to something less
missleading.

>       In summary, the branch must be the first entry of the
>       parallel (also required by jump.c), and the second
>       entry of the parallel must be a set of the loop counter
> !      register.
> !
> !      The code was extended to recognize a decrement followed
> !      by branch_ne sequence of patterns:
> !       
> !       (set (reg) (plus (reg) (const_int -1))
> !       (set (pc) (if_then_else (reg != 0)
> !                               (label_ref (label))
> !                               (pc))).  */

I would suggest a different wording of this comment:  "The canonical doloop
pattern we expect has one of the following forms:

1) ... parallel...
   The branch must be the first entry of the parallel (also required by
   jump.c), and the second entry of the parallel must be a set of the loop counter
   register.  Some targets (IA-64) wrap the set of the loop counter in
   an if_then_else too.

2) ... the sequence of two insns...

> static void
> doloop_modify (struct loop *loop, struct niter_desc *desc,
>!              rtx doloop_seq, rtx condition, rtx count, bool inc_first)

Instead of introducing INC_FIRST, you should make doloop_condition_get
return the condition (reg != 1) -- doloop_modify already handles this case.

> +   /* Get the probabilty of the original branch. It may be reversed
> +      but the REG_BR_PROB was not updated.  */
> +   true_prob_val = find_reg_note (jump_insn, REG_BR_PROB, NULL_RTX);
> + 

This comment is quite missleading, as you actually never use the value
of true_prob_val.

> +   /* Update the REG_BR_PROB note.  */
> +   if (inc_first && true_prob_val)
> +     {

Why do you update the REG_BR_PROB note (only) in case inc_first is true?
The code emitted with inc_first does not actually modify the jumps,
so the REG_BR_PROB should either be added always or not at all.

Zdenek