This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Modulo-scheduling improvements. Patch 2 of 2.
- From: Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>
- To: volodyan at gmail dot com
- Cc: ZAKS at il dot ibm dot com, Andrey Belevantsev <abel at ispras dot ru>, Dorit Nuzman <dorit at il dot ibm dot com>, gcc-patches at gcc dot gnu dot org, Mircea Namolaru <NAMOLARU at il dot ibm dot com>, vmakarov at toronto dot redhat dot com, David Edelsohn <dje at watson dot ibm dot com>
- Date: Thu, 21 Jun 2007 23:29:32 +0200
- Subject: Re: [PATCH] Modulo-scheduling improvements. Patch 2 of 2.
- References: <48ee23ef0706190510o2cb974bco9e8664ae7f73730c@mail.gmail.com> <OFA422CFCB.1CE8002F-ONC2257300.0073C7DD-C2257300.0076BBB5@il.ibm.com>
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