Hi, Richi. >> that should be
>> || (!LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) >> && !LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)) >> I think. It seems to imply that SLP isn't supported with >> masking/lengthing. Oh, yes. At first glance, the original code is quite suspicious and your comments make sense to me. >> Hum, how does CFN_EXTRACT_LAST handle both mask and length transparently? >> Don't you need some CFN_LEN_EXTRACT_LAST instead? I think CFN_EXTRACT_LAST always has either loop mask or loop len. When both mask and length are not needed, IMHO, I think current BIT_FIELD_REF flow is good enough: https://godbolt.org/z/Yr5M9hcc6 So I think we don't need CFN_LEN_EXTRACT_LAST. Instead, I think we will need CFN_LEN_FOLD_EXTRACT_LAST in the next patch. Feel free to correct me it I am wrong. Thanks. juzhe.zh...@rivai.ai From: Richard Biener Date: 2023-08-09 19:00 To: Ju-Zhe Zhong CC: gcc-patches; richard.sandiford Subject: Re: [PATCH] VECT: Support loop len control on EXTRACT_LAST vectorization On Wed, 9 Aug 2023, juzhe.zh...@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zh...@rivai.ai> > > Hi, this patch is adding loop len control on extract_last autovectorization. > > Consider this following case: > > #include <stdint.h> > > #define EXTRACT_LAST(TYPE) \ > TYPE __attribute__ ((noinline, noclone)) \ > test_##TYPE (TYPE *x, int n, TYPE value) \ > { \ > TYPE last; \ > for (int j = 0; j < n; ++j) \ > { \ > last = x[j]; \ > x[j] = last * value; \ > } \ > return last; \ > } > > #define TEST_ALL(T) \ > T (uint8_t) \ > > TEST_ALL (EXTRACT_LAST) > > ARM SVE IR: > > Preheader: > max_mask_34 = .WHILE_ULT (0, bnd.5_6, { 0, ... }); > > Loop: > ... > # loop_mask_22 = PHI <next_mask_35(4), max_mask_34(3)> > ... > vect_last_12.8_23 = .MASK_LOAD (_7, 8B, loop_mask_22); > vect__4.9_27 = vect_last_12.8_23 * vect_cst__26; > .MASK_STORE (_7, 8B, loop_mask_22, vect__4.9_27); > ... > next_mask_35 = .WHILE_ULT (_1, bnd.5_6, { 0, ... }); > ... > > Epilogue: > _25 = .EXTRACT_LAST (loop_mask_22, vect_last_12.8_23); > > For RVV since we prefer len in loop control, after this patch for RVV: > > Loop: > ... > loop_len_22 = SELECT_VL; > vect_last_12.8_23 = .MASK_LOAD (_7, 8B, loop_len_22); > vect__4.9_27 = vect_last_12.8_23 * vect_cst__26; > .MASK_STORE (_7, 8B, loop_len_22, vect__4.9_27); > ... > > Epilogue: > _25 = .EXTRACT_LAST (loop_len_22, vect_last_12.8_23); > > This patch didn't add a new pattern for length loop control of extract_last. > Instead we reuse current extract_last. > > Here is the code: > > Step 1 - Enable length and record length for extract_last: > > + machine_mode vec_mode = TYPE_MODE (vectype); > + if (get_len_load_store_mode (vec_mode, true).exists (&vec_mode)) > + vect_record_loop_len (loop_vinfo, > + &LOOP_VINFO_LENS (loop_vinfo), 1, > + vectype, 1); > + else > + vect_record_loop_mask (loop_vinfo, > + &LOOP_VINFO_MASKS (loop_vinfo), 1, > + vectype, NULL); > > We use 'get_len_load_store_mode' to check whether targets support loop len > control or not. > If yes, record a loop len. > > Step 2 - Build EXTRACT_LAST with len: > > - tree mask = vect_get_loop_mask (loop_vinfo, gsi, > - &LOOP_VINFO_MASKS (loop_vinfo), > - 1, vectype, 0); > + tree control; > + if (LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)) > + control = vect_get_loop_len (loop_vinfo, gsi, > + &LOOP_VINFO_LENS (loop_vinfo), 1, > + vectype, 0, 0); > + else > + control = vect_get_loop_mask (loop_vinfo, gsi, > + &LOOP_VINFO_MASKS (loop_vinfo), 1, > + vectype, 0); > tree scalar_res = gimple_build (&stmts, CFN_EXTRACT_LAST, scalar_type, > - mask, vec_lhs_phi); > + control, vec_lhs_phi); > > Reuse the current codes (build EXTRACT_LAST with mask), build length instead > if > 'LOOP_VINFO_FULLY_WITH_LENGTH_P' is true. > > This patch has been fully tested in RISC-V port. > > Bootstrap and Regression on X86 passed. > > Ok for trunk ? > > gcc/ChangeLog: > > * tree-vect-loop.cc (vectorizable_live_operation): Add length control. > > --- > gcc/tree-vect-loop.cc | 40 ++++++++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > index 00058c3c13e..fde098cafde 100644 > --- a/gcc/tree-vect-loop.cc > +++ b/gcc/tree-vect-loop.cc > @@ -10311,9 +10311,15 @@ vectorizable_live_operation (vec_info *vinfo, > else > { > gcc_assert (ncopies == 1 && !slp_node); > - vect_record_loop_mask (loop_vinfo, > - &LOOP_VINFO_MASKS (loop_vinfo), > - 1, vectype, NULL); > + machine_mode vec_mode = TYPE_MODE (vectype); > + if (get_len_load_store_mode (vec_mode, true).exists (&vec_mode)) > + vect_record_loop_len (loop_vinfo, > + &LOOP_VINFO_LENS (loop_vinfo), 1, > + vectype, 1); > + else > + vect_record_loop_mask (loop_vinfo, > + &LOOP_VINFO_MASKS (loop_vinfo), 1, > + vectype, NULL); > } > } > /* ??? Enable for loop costing as well. */ > @@ -10339,7 +10345,9 @@ vectorizable_live_operation (vec_info *vinfo, > gimple *vec_stmt; > if (slp_node) > { > - gcc_assert (!loop_vinfo || !LOOP_VINFO_FULLY_MASKED_P (loop_vinfo)); > + gcc_assert (!loop_vinfo > + || !LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) > + || !LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)); that should be || (!LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) && !LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)) I think. It seems to imply that SLP isn't supported with masking/lengthing. > > /* Get the correct slp vectorized stmt. */ > vec_lhs = SLP_TREE_VEC_DEFS (slp_node)[vec_entry]; > @@ -10383,21 +10391,29 @@ vectorizable_live_operation (vec_info *vinfo, > > gimple_seq stmts = NULL; > tree new_tree; > - if (LOOP_VINFO_FULLY_MASKED_P (loop_vinfo)) > + if (LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) > + || LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)) > { > /* Emit: > > - SCALAR_RES = EXTRACT_LAST <VEC_LHS, MASK> > + SCALAR_RES = EXTRACT_LAST <VEC_LHS, CONTROL> > > - where VEC_LHS is the vectorized live-out result and MASK is > - the loop mask for the final iteration. */ > + where VEC_LHS is the vectorized live-out result and CONTROL can > + be either the loop mask for the final iteration or the loop len > + for the final iteration. */ > gcc_assert (ncopies == 1 && !slp_node); > tree scalar_type = TREE_TYPE (STMT_VINFO_VECTYPE (stmt_info)); > - tree mask = vect_get_loop_mask (loop_vinfo, gsi, > - &LOOP_VINFO_MASKS (loop_vinfo), > - 1, vectype, 0); > + tree control; > + if (LOOP_VINFO_FULLY_WITH_LENGTH_P (loop_vinfo)) > + control = vect_get_loop_len (loop_vinfo, gsi, > + &LOOP_VINFO_LENS (loop_vinfo), 1, > + vectype, 0, 0); > + else > + control = vect_get_loop_mask (loop_vinfo, gsi, > + &LOOP_VINFO_MASKS (loop_vinfo), 1, > + vectype, 0); > tree scalar_res = gimple_build (&stmts, CFN_EXTRACT_LAST, scalar_type, > - mask, vec_lhs_phi); > + control, vec_lhs_phi); Hum, how does CFN_EXTRACT_LAST handle both mask and length transparently? Don't you need some CFN_LEN_EXTRACT_LAST instead? > /* Convert the extracted vector element to the scalar type. */ > new_tree = gimple_convert (&stmts, lhs_type, scalar_res); > Otherwise looks OK to me. Thanks, Richard.
