Re: [SVE] PR86753

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Aug 25, 2019 at 11:13 PM Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> On Fri, 23 Aug 2019 at 19:43, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > 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.)
> Hi,
> Does the attached patch look OK ?
> In patch, I put call to vn_visit stmt in bb loop in
> vect_transform_loop to avoid replicating logic for processing phi and
> stmts.
> AFAIU, vect_transform_loop_stmt is only called from bb loop, so
> compile time penalty for checking cond_to_vec_mask
> should be pretty small ?
> If this is not OK, I will walk bb immediately before the bb loop.

So if I understand correctly you never have vectorizable COND_EXPRs
in SLP mode?  Because we vectorize all SLP chains before entering
the loop in vect_transform_loop where you VN existing scalar(!) stmts.

Then all this hew hash-table stuff should not be needed since this
is what VN should provide you with.  You of course need to visit
generated condition stmts.  And condition support is weak
in VN due to it possibly having two operations in a single stmt.
Bad GIMPLE IL.  So I'm not sure VN is up to the task here or
why you even need it given you are doing your own hashing?

Richard.

> Thanks,
> Prathamesh
> >
> > Thanks,
> > Richard