This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Lno branch merge part 12 -- induction variable optimizations
Hello,
> >1) does not apply in some cases, especially if we obtained the
> >statement
> >as a statement that defines a given ssa name.
> >
> >>If you think, performance implications are not considerable
> >
> >The performance definitely might be a problem, eventually. I haven't
> >seen the problems occur in practice yet, but I think that we should
> >have
> >a way how to obtain a bsi for statement in a constant time (but this is
> >something for a separate patch).
> >
> >>than make remove_statement() also externally visible :-).
> >
> >Why?
>
> see above.
what exactly you mean? I see nothing above which would justify
exporting remove_statement.
> > tree-ssa-loop-ivopts.c:remove_statement is a function that is
> >fairly specific for what ivopts need, especially handling of ssa names
> >in phi nodes is specifically tailored for it. I.e. I do not think it
> >is a good idea for other optimizers to use it.
> >
> >>Some of the new functions, e.g. force_gimple_operand(),
> >>add_candidate_1(), get_address_cost(), does not have any comments
> >>inside at all. And they are not small. Comments would be helpful to
> >>someone down the road after few months.
> >
> >They are not small, but fairly straightforward (except for
> >get_address_cost, which could definititely use some comments);
> >it is hard to come up with comments that are not obvious
> >in force_gimple_operand and add_candidate_1.
> >
> >>>+ /* Per-ssa version information (induction variable descriptions,
> >>>etc.). */
> >>>+ struct version_info
> >>>+ {
> >>
> >>use prefix iv, iv_version_info ?
> >
> >Why? It would just make names longer.
>
> other passes may want to use loop_data, version_info etc.. Using prefix
> makes it cleaner.
>
> >
> >>>+ /* Information attached to loop. */
> >>>+ struct loop_data
> >>
> >>same. few other structs also.
> >>
> >>You are already use iv prefix for some of them, e.g. iv_use, iv_cand
> >>etc..
> >
> >Yes, for those that have "induction variable" as part of name; e.g.
> >"induction variable use" and "induction variable candidate".
>
> That's my point. Replacing 'loop_data' with 'iv_loop_data' or
> 'ivopts_loop_data' is more meaningful.
iv_loop_data would read as "induction variable loop data", which is
nonsense. I will consider ivopts_loop_data, but I am not really
persuaded it helps anything.
> >>>+ /* Returns cost of multiplication by constant CST in MODE. */
> >>>+
> >>>+ static unsigned
> >>>+ multiply_by_cost (HOST_WIDE_INT cst, enum machine_mode mode)
> >>
> >>rename as get_multiplication_cost() ?
> >>No, comments explaining how the cost is calculated.
> >
> >??? What would you like to write there?
>
> You are intimately familiar with this code so everything is obvious for
> you.
No I am not. I wrote the multiply_by_cost function 6 months ago, so I no
longer recalled anything about its inner workings when I looked whether
I can do anything with your request to add a comment to it. It is just
that trivial that I do not think a comment would make it any cleaner
(unlike say get_address_cost, which I will spend some time over before
I decipher what it really does, so that I may add comments).
> It is always better to write something than relay on other person
> to read your mind.
>
> > We check whether the result
> >is not cached, and if it is not, we expand the multiplication to
> >rtl and determine the cost.
>
> Even this is useful. 100 seems like a "odd" (:-) number for initial
> size of hash table.
It is just an ad hoc number, no more "odd" than any other size I might
put there.
Zdenek