Re: IVOPT improvement patch

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Tue, May 11, 2010 at 8:35 AM, Xinliang David Li <davidxl@google.com> wrote:
> The attached patch handles all the problems above except for 7.
>
>
> Bootstrapped and regression tested on linux/x86_64.
>
> The patch was not tuned for SPEC, but SPEC testing was done.
> Observable improvements : gcc 4.85%, vpr 1.53%, bzip2 2.36%, and eon
> 2.43% (Machine CPU: Intel Xeon E5345/2.33Ghz, m32mode).

Impressive!

Could you please split the patch into smaller bits, explaining for
each part of the patch what it does?  It is very difficult to review
large patches like this. You should also commit the patch per part
once approved -- it's also very difficult to hunt regressions down to
single changes when the single change is very large.

Some stylistic comments:

> +  /* Pseudo version infos for generated loop invariants.  */
> +  VEC(version_info_p,heap) *pseudo_version_info;

Could you explain what you mean with "pseudo invariants"? Most GCC
developers will think of pseudo-registers when they see the term
pseudo in GCC, but I think you use the word in a different context
here. Best would be to use another term (dummy?) or otherwise add an
explanation.


> +#define PSEUDO_COMMON_FACTOR 0.3

We try to avoid floating point math in GCC (portability issues). You
should use integer math instead (e.g. see what GCC does with
REG_BR_PROB_BASE, and CGRAPH_FREQ_BASE, and in a few other places).

>  /* Finds addresses in *OP_P inside STMT.  */
>
> -static void
> +static bool
> find_interesting_uses_address (struct ivopts_data *data, gimple stmt, tree *op_p)

Please document the return value.


> -find_interesting_uses_op (struct ivopts_data *data, tree op)
> +find_interesting_uses_op (struct ivopts_data *data, tree op, enum iv_use_pos use_pos)

Please keep the line lengths at less than 80 characters.

> +adjust_iv_update_pos (struct ivopts_data *data ATTRIBUTE_UNUSED, struct iv_cand *cand)

And here, too... And elsewhere, where I didn't spot it :-)

Ciao!
Steven