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.
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)
