Richard Biener <rguent...@suse.de> writes: > 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
Brutal but true :P > 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. Fair enough. I'm about halfway through fixing the issues we see (full disclosure, the first half is the easy half). And about the need to at least guard the problematic code appropriately, that's what I've done wherever I've not yet developed a satisfactory solution. Still, a full fix would be nice. I look forward to submitting the relevant fixes. Many thanks, Victor > 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; >> >> >> > >> >>
