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