On Wed, 12 Jul 2023, juzhe.zh...@rivai.ai wrote:
> Thanks Richard.
>
> Is it correct that the better way is to add optabs
> (len_strided_load/len_strided_store),
> then expand LEN_MASK_GATHER_LOAD/LEN_MASK_SCATTER_STORE to
> len_strided_load/len_strided_store optab (if it is strided load/store) in
> expand_gather_load_optab_fn
> expand_scatter_store_optab_fn
>
> of internal-fn.cc
>
> Am I right? Thanks.
Yes.
In priciple the vectorizer can also directly take advantage of this
and code generate an internal .LEN_STRIDED_LOAD ifn.
Richard.
> juzhe.zh...@rivai.ai
>
> From: Richard Biener
> Date: 2023-07-12 15:27
> To: juzhe.zh...@rivai.ai
> CC: jeffreyalaw; gcc-patches; Kito.cheng; Robin Dapp; richard.sandiford
> Subject: Re: Re: [PATCH V5] RISC-V: Support gather_load/scatter RVV
> auto-vectorization
> On Wed, 12 Jul 2023, juzhe.zh...@rivai.ai wrote:
>
> > I understand your concern. I CC Richards to see whether this piece of codes
> > is unsafe.
> >
> > Hi, Richard and Richi:
> >
> > Jeff is worrying about this codes in "expand_gather_scatter" of supporting
> > len_mask_gather_load/len_mask_scatter_store in RISC-V port.
> >
> > The codes are as follows:
> >
> > +/* Return true if it is the strided load/store. */
> > +static bool
> > +strided_load_store_p (rtx vec_offset, rtx *base, rtx *step)
> > +{
> > + if (const_vec_series_p (vec_offset, base, step))
> > + return true;
> > +
> > + /* For strided load/store, vectorizer always generates
> > + VEC_SERIES_EXPR for vec_offset. */
> > + tree expr = REG_P (vec_offset) ? REG_EXPR (vec_offset) : NULL_TREE;
> > + if (!expr || TREE_CODE (expr) != SSA_NAME)
> > + return false;
> > +
> > + /* Check if it is GIMPLE like: _88 = VEC_SERIES_EXPR <0, _87>; */
> > + gimple *def_stmt = SSA_NAME_DEF_STMT (expr);
> > + if (!def_stmt || !is_gimple_assign (def_stmt)
> > + || gimple_assign_rhs_code (def_stmt) != VEC_SERIES_EXPR)
> > + return false;
> > +
> > + tree baset = gimple_assign_rhs1 (def_stmt);
> > + tree stept = gimple_assign_rhs2 (def_stmt);
> > + *base = expand_normal (baset);
> > + *step = expand_normal (stept);
> > +
> > + if (!rtx_equal_p (*base, const0_rtx))
> > + return false;
> > + return true;
> > +}
> > In this codes, I tried to query the SSA_NAME_DEF_STMT to see whether the
> > vector offset of gather/scatter is VEC_SERISE
> > If it is VEC_SERISE, I will lower them into RVV strided load/stores
> > (vlse.v/vsse.v) which is using scalar stride,
> > if it is not, then use common RVV indexed load/store with vector offset
> > (vluxei/vsuxei).
> >
> > Jeff is worrying about whether we are using SSA_NAME_DEF_STMT at this point
> > (during the stage "expand" expanding gimple ->rtl).
>
> Using SSA_NAME_DEF_STMT during expansion is OK, but I don't think you
> can rely on REG_EXPR here since you don't know whether any coalescing
> happened. That said, maybe the implementation currently guarantees
> you'll only see a REG_EXPR SSA name if there's a single definition
> of that register, but at least I'm not aware of that and this is also
> not documented.
>
> I wonder if you can recover vlse.v at combine time though?
>
> That said, if the ISA supports gather/scatter with an affine offset
> the more appropriate way would be to add additional named expanders
> for this and deal with the above in the middle-end during RTL
> expansion instead.
>
> Richard.
>
> > I am also wondering whether I am doing wrong here.
> > Thanks.
> >
> >
> > juzhe.zh...@rivai.ai
> >
> > From: Jeff Law
> > Date: 2023-07-12 13:32
> > To: juzhe.zh...@rivai.ai; gcc-patches
> > CC: Kito.cheng; Robin Dapp
> > Subject: Re: [PATCH V5] RISC-V: Support gather_load/scatter RVV
> > auto-vectorization
> >
> >
> > On 7/11/23 20:34, juzhe.zh...@rivai.ai wrote:
> > > Hi, Jeff.
> > >
> > > >> Hmm, I'm not sure this is safe, especially if gimple->rtl expansion is
> > >>>complete. While you might be able to get REG_EXPR, I would not really
> > >>>expect SSA_NAME_DEF_STMT to be correct. At the least it'll need some
> > >>>way to make sure it's not called at an inappropriate time.
> > > I think it's safe, if SSA_NAME_DEF_STMT is NULL, then just return it.
> > >
> > >>>Should this have been known_lt rather than known_le?
> > > It should be LE, since I will pass through GET_MODE_NUNITS/GET_MODE_SIZE
> > > for SLP.
> > THanks for double checking. It looked slightly odd checking ge or le.
> >
> >
> > >
> > >>>Something's off in your formatting here. I'd guess spaces vs tabs
> > > Ok.
> > >
> > >>>In a few places you're using expand_binop. Those interfaces are really
> > >>>more for gimple->RTL. BUt code like expand_gather_scatter is really
> > >>>RTL, not gimple/tree. Is there a reason why you're not using pure RTL
> > >>>interfaces?
> > > I saw ARM SVE is using them in many places for expanding patterns.
> > > And I think it's convenient so that's why I use them.
> > OK.
> >
> > I still think we need a resolution on strided_load_store_p. As I
> > mentioned in my original email, I'm not sure you can depend on getting
> > to the SSA_NAME_DEF_STMT at this point -- in particular if it's a
> > dangling pointer, then bad things are going to happen. So let's chase
> > that down. Presumably this is called during gimple->rtl expansion,
> > right? Is it ever called later?
> >
> > I think my concerns about expand_gather_scatter are a non-issue after
> > looking at it again -- I missed the GET_MODE (step) != Pmode conditional
> > when I first looked at that code.
> >
> >
> > jeff
> >
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
