On Wed, Mar 1, 2023 at 10:00 PM Michael Collison <colli...@rivosinc.com> wrote:
>
> Okay there seems to be consensus on using constant_lower_bound (vf), but
> I don't understand how that is a replacement for "vf.is_constant ()"? In
> one case we are checking if "vf" is a constant, on the other we are
> asking for the lower bound. For the crash in question
> "constant_lower_bound (vf) " returns the integer value of two.
Use the result of constant_lower_bound (vf) in place of 'vf' when
build_int_cst. That should be correct for both poly and non-poly int VF.
> On 2/27/23 09:51, Richard Sandiford wrote:
> > FWIW, this patch looks good to me. I'd argue it's a regression fix
> > of kinds, in that the current code was correct before variable VF and
> > became incorrect after variable VF. It might be possible to trigger
> > the problem on SVE too, with a sufficiently convoluted test case.
> > (Haven't tried though.)
> >
> > Richard Biener <richard.guent...@gmail.com> writes:
> >> On Wed, Feb 22, 2023 at 12:03 AM Michael Collison <colli...@rivosinc.com>
> >> wrote:
> >>> While working on autovectorizing for the RISCV port I encountered an
> >>> issue where vect_do_peeling assumes that the vectorization factor is a
> >>> compile-time constant. The vectorization is not a compile-time constant
> >>> on RISCV.
> >>>
> >>> Tested on RISCV and x86_64-linux-gnu. Okay?
> >> I wonder how you arrive at prologue peeling with a non-constant VF?
> > Not sure about the RVV case, but I think it makes sense in principle.
> > E.g. if some ISA takes the LOAD_LEN rather than fully-predicated
> > approach, it can't easily use the first iteration of the vector loop
> > to do peeling for alignment. (At least, the IV steps would then
> > no longer match VF for all iterations.) I guess it could use a
> > *different* vector loop, but we don't support that yet.
> >
> > There are also some corner cases for which we still don't support
> > predicated loops and instead fall back on an unpredicated VLA loop
> > followed by a scalar epilogue. Peeling for alignment would then
> > require a scalar prologue too.
> >
> >> In any case it would probably be better to use constant_lower_bound (vf)
> >> here? Also it looks wrong to apply this limit in case we are using
> >> a fully masked main vector loop. But as said, the specific case of
> >> non-constant VF and prologue peeling probably wasn't supposed to happen,
> >> instead the prologue usually is applied via an offset to a fully masked
> >> loop?
> > Hmm, yeah, agree constant_lower_bound should work too.
> >
> > Thanks,
> > Richard
> >
> >> Richard?
> >>
> >> Thanks,
> >> Richard.
> >>
> >>> Michael
> >>>
> >>> gcc/
> >>>
> >>> * tree-vect-loop-manip.cc (vect_do_peeling): Verify
> >>> that vectorization factor is a compile-time constant.
> >>>
> >>> ---
> >>> gcc/tree-vect-loop-manip.cc | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> >>> index 6aa3d2ed0bf..1ad1961c788 100644
> >>> --- a/gcc/tree-vect-loop-manip.cc
> >>> +++ b/gcc/tree-vect-loop-manip.cc
> >>> @@ -2930,7 +2930,7 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree
> >>> niters, tree nitersm1,
> >>> niters = vect_build_loop_niters (loop_vinfo, &new_var_p);
> >>> /* It's guaranteed that vector loop bound before vectorization
> >>> is at
> >>> least VF, so set range information for newly generated var. */
> >>> - if (new_var_p)
> >>> + if (new_var_p && vf.is_constant ())
> >>> {
> >>> value_range vr (type,
> >>> wi::to_wide (build_int_cst (type, vf)),
> >>> --
> >>> 2.34.1
> >>>
