Re: [SVE] PR86753

[Richard Sandiford <richard.sandiford@arm.com>]

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> On Fri, 23 Aug 2019 at 18:15, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> > On Thu, 22 Aug 2019 at 16:44, Richard Biener <richard.guenther@gmail.com> wrote:
>> >> It looks a bit odd to me.  I'd have expected it to work by generating
>> >> the stmts as before in the vectorizer and then on the stmts we care
>> >> invoke vn_visit_stmt that does both value-numbering and elimination.
>> >> Alternatively you could ask the VN state to generate the stmt for
>> >> you via vn_nary_build_or_lookup () (certainly that needs a bit more
>> >> work).  One complication might be availability if you don't value-number
>> >> all stmts in the block, but well.  I'm not sure constraining to a single
>> >> block is necessary - I've thought of having a "CSE"ing gimple_build
>> >> for some time (add & CSE new stmts onto a sequence), so one
>> >> should keep this mode in mind when designing the one working on
>> >> an existing BB.  Note as you write it it depends on visiting the
>> >> stmts in proper order - is that guaranteed when for example
>> >> vectorizing SLP?
>> > Hi,
>> > Indeed, I wrote the function with assumption that, stmts would be
>> > visited in proper order.
>> > This doesn't affect SLP currently, because call to vn_visit_stmt in
>> > vect_transform_stmt is
>> > conditional on cond_to_vec_mask, which is only allocated inside
>> > vect_transform_loop.
>> > But I agree we could make it more general.
>> > AFAIU, the idea of constraining VN to single block was to avoid using defs from
>> > non-dominating scalar stmts during outer-loop vectorization.
>>
>> Maybe we could do the numbering in a separate walk immediately before
>> the transform phase instead.
> Um sorry, I didn't understand. Do you mean we should do dom based VN
> just before transform phase
> or run full VN ?

No, I just meant that we could do a separate walk of the contents
of the basic block:

> @@ -8608,6 +8609,8 @@ vect_transform_loop (loop_vec_info loop_vinfo)
>      {
>        basic_block bb = bbs[i];
>        stmt_vec_info stmt_info;
> +      vn_bb_init (bb);
> +      loop_vinfo->cond_to_vec_mask = new cond_vmask_map_type (8);
> 

...here, rather than doing it on the fly during vect_transform_stmt
itself.  The walk should be gated on LOOP_VINFO_FULLY_MASKED_P so that
others don't have to pay the compile-time penalty.  (Same for
cond_to_vec_mask itself really.)

Thanks,
Richard