Re: [PATCH] Unrolling addressing optimization

[Revital Eres <ERES at il dot ibm dot com>]

Hello,

Thanks for your comments.

Concerning the -fweb optimization - my understanding is that -fweb 
implements different optimization. 
Indeed I tried some simple examples and did not get the desired code.

Revital







Zdenek Dvorak <rakdver@atrey.karlin.mff.cuni.cz>
07/04/2004 13:40
 
        To:     Revital Eres/Haifa/IBM@IBMIL
        cc:     gcc-patches@gcc.gnu.org
        Subject:        Re: [PATCH] Unrolling addressing optimization


Hello,

> During the unrolling we do the following transformation
> for array accesses (loads and stores):
> 
> After the unrolling for an access to an array ("a[i]")
> we have the following pattern:
> 
>     i = i + 1
>     load a[i]
>     ....
>     i = i + 1
>     load a[i]
>     ....
>     i = i + 1
>     load a[i]
>     ....
>     i = i + 1
>     load a[i]
> 
> Our optimization computes the address of a[i] (addr =&a[i])
> and we have the following code:
> 
>    load addr
>    ....
>    load 4(addr)
>    ....
>    load 8(addr)
>    ....
>    load 12(addr) 
>    ... 

this optimization is already done if you use -fweb.
I do not know whether there is a reason to duplicate this optimization,
but anyway some comments to the patch:

> *************** end:
> *** 1254,1260 ****
>   void
>   copy_bbs (basic_block *bbs, unsigned n, basic_block *new_bbs,
>           edge *edges, unsigned n_edges, edge *new_edges,
> !         struct loop *base)
>   {
>     unsigned i, j;
>     basic_block bb, new_bb, dom_bb;
> --- 1265,1273 ----
>   void
>   copy_bbs (basic_block *bbs, unsigned n, basic_block *new_bbs,
>                  edge *edges, unsigned n_edges, edge *new_edges,
> !                struct loop *base,
> !                struct loop_reg_info* iv_regs, struct loop_insn_info* 
> loop_insns_info,
> !                        unsigned iter, unsigned ndup)
>   {
>     unsigned i, j;
>     basic_block bb, new_bb, dom_bb;

Keep copy_bbs generic (i.e. independent on the particular optimization
it is used from) -- add a generic callback function argument to it,
set to handle_ivs by caller.

Ditto for duplicate_loop_to_header_edge.

I would also suggest renaming handle_ivs to something like
split_ivs_during_unrolling, since "handle_ivs" might mean anything.

> + static bool
> + can_split_insn(struct loop_insn_info* loop_insns_info, 
> +                struct loop_reg_info* loop_regs_info,
> +                rtx* insn, int insn_num)

Coding style:
  missing space before "("
  rtx* insn ==> rtx *insn   (occurs quite often in the patch)

> + static void mark_modified_reg (struct loop_reg_info * , rtx);
> + static void analyze_loop_invariants (struct loop_reg_info *, 
> +                                          basic_block * , int);
> + static void find_shift_pattern_in_loop (struct loop *, 
> +                                         struct loop_reg_info *, 
> +                                         basic_block *, int);
> + static void find_shift_giv (struct loop_reg_info * ,rtx);

Coding style -- no space before ",".

I am somewhat unsure why you need these functions.  What you do
is replacing mem[addr] by mem[addr + step * "copy number"]; all what you 
need
to know for this is that addr is an induction variable with known step;
you should not need to care about invariants, whether it is biv/shifted
biv, etc.

We currently do not handle shifts in loop-iv.c; if this is the reason
you are adding these here, you should add this to the induction variable
analysis instead.

> + bool
> + is_biv (int regno)
> + {
> +   if (!bivs)
> +     return false;
> +   if(!bivs[regno].analysed)
> +     return false;
> +   return true;
> + }

You probably want also check bivs[regno]->base != NULL here (this just
checks whether we happened to test whether regno is a biv, not that we
succeeded with it).

Zdenek