Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> writes:
> [...]
>> > + {
>> > + struct mem_address parts = {NULL_TREE, integer_one_node,
>> > + NULL_TREE, NULL_TREE, NULL_TREE};
>>
>> Might be better to use "= {}" and initialise the fields that matter by
>> assignment. As it stands this uses integer_one_node as the base, but I
>> couldn't tell if that was deliberate.
>
> I just copied this part from get_address_cost, similar to what is done
> there.
Ah, sorry :-)
> I have now changed the way you suggested but using the values
> used in get_address_cost.
Thanks.
> [...]
> @@ -3479,6 +3481,35 @@ add_iv_candidate_derived_from_uses (struct ivopts_data
> *data)
> data->iv_common_cands.truncate (0);
> }
>
> +/* Return the preferred mem scale factor for accessing MEM_MODE
> + of BASE in LOOP. */
> +static unsigned int
> +preferred_mem_scale_factor (struct loop *loop,
> + tree base, machine_mode mem_mode)
IMO this should live in tree-ssa-address.c instead.
The only use of "loop" is to test for size vs. speed, but other callers
might want to make that decision based on individual blocks, so I think
it would make sense to pass a "speed" bool instead. Probably also worth
making it the last parameter, so that the order is consistent with
address_cost (though probably then inconsistent with something else :-)).
> [...]
> @@ -3500,6 +3531,28 @@ add_iv_candidate_for_use (struct ivopts_data *data,
> struct iv_use *use)
> basetype = sizetype;
> record_common_cand (data, build_int_cst (basetype, 0), iv->step, use);
>
> + /* Compare the cost of an address with an unscaled index with the cost of
> + an address with a scaled index and add candidate if useful. */
> + if (use != NULL
> + && poly_int_tree_p (iv->step)
> + && tree_fits_poly_int64_p (iv->step)
> + && address_p (use->type))
> + {
> + poly_int64 new_step;
> + poly_int64 poly_step = tree_to_poly_int64 (iv->step);
This should be:
poly_int64 step;
if (use != NULL
&& poly_int_tree_p (iv->step, &step)
&& address_p (use->type))
{
poly_int64 new_step;
> + unsigned int fact
> + = preferred_mem_scale_factor (data->current_loop,
> + use->iv->base,
> + TYPE_MODE (use->mem_type));
> +
> + if ((fact != 1)
> + && multiple_p (poly_step, fact, &new_step))
Should be no brackets around "fact != 1".
> [...]
Looks really good to me otherwise, thanks. Bin, any comments?
Richard
