On Mon, 18 Nov 2024, Victor Do Nascimento wrote: > On 11/5/24 07:39, Richard Biener wrote: > > On Tue, 5 Nov 2024, Victor Do Nascimento wrote: > > > >> The current codegen code to support VF's that are multiples of a simdclone > >> simdlen rely on BIT_FIELD_REF to create multiple input vectors. This does > >> not > >> work for non-constant simdclones, so we should disable using such clones > >> when > >> the VF is a multiple of the non-constant simdlen until we change the > >> codegen to > >> support those. > > > > ISTR BIT_FIELD_REF now uses poly-int offset and size so what breaks > > here? I don't see any other way that such BIT_FIELD_REFs to represent > > hi/lo part accesses? > > > > Richard. > > Upon further investigation, while you are right that BIT_FIELD_REF does > use poly-int types for both offsets and sizes, much of the expand code > does not know how to deal with variable-len vectors. > > One such example of this is in `store_constructor_field' where, after > having called our simdclone fn twice passing it the high and low parts > of an SVE vec as an argument, we try to concatenate the returned > subvectors back together. > > In `store_constructor', for example, many updates from integer operators > to their poly_int64 counterparts are needed (though this is trivial) and > in other parts new logic is needed altogether. > > One example of this can be seen in `store_bit_field_1', where you see > the following statments: > > unsigned HOST_WIDE_INT ibitsize = bitsize.to_constant (); > unsigned HOST_WIDE_INT ibitnum = bitnum.to_constant (); > > which will ICE for variable-length bitsizes and bitnums and which guard > the subsequent section of code from trying to handle non-const lens.
But since BIT_FIELD_REFs with poly-int offset/size are now a thing we have to fix all those places. Or revert back to not allowing them. We at least need to guard the problematic code appropriately. > Therefore, while I believe that the full support for poly-int > BIT_FIELD_REFs is appropriate and something that ought to be done, the > > if (!n->simdclone->simdlen.is_constant () && num_calls != 1) > > guard in this patch is, at least for now, strictly necessary and can be > readily removed once work on BIT_FIELD_REF is completed. GCCs history tells us this will never happen ;) Given this is for a new feature I insist you try a bit harder and fixup the places that ICE (if only by guarding them with .is_constant ()). Those parts will be helpful even if in the end you don't succeed with bug-squashing. Richard. > Regards, > Victor > > >> gcc/ChangeLog: > >> > >> * tree-vect-stmts.cc (vectorizable_simd_clone_call): Reject simdclones > >> with non-constant simdlen when VF is not exactly the same. > >> --- > >> gcc/tree-vect-stmts.cc | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > >> index 2d0da6f0a0e..961421fee25 100644 > >> --- a/gcc/tree-vect-stmts.cc > >> +++ b/gcc/tree-vect-stmts.cc > >> @@ -4149,7 +4149,10 @@ vectorizable_simd_clone_call (vec_info *vinfo, > >> stmt_vec_info stmt_info, > >> if (!constant_multiple_p (vf * group_size, n->simdclone->simdlen, > >> &num_calls) > >> || (!n->simdclone->inbranch && (masked_call_offset > 0)) > >> - || (nargs != simd_nargs)) > >> + || (nargs != simd_nargs) > >> + /* Currently we do not support multiple calls of non-constant > >> + simdlen as poly vectors can not be accessed by BIT_FIELD_REF. > >> */ > >> + || (!n->simdclone->simdlen.is_constant () && num_calls != 1)) > >> continue; > >> if (num_calls != 1) > >> this_badness += floor_log2 (num_calls) * 4096; > >> > > > > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
