Re: [PATCH] vect: Check that vector factor is a compile-time constant

[Richard Sandiford <richard.sandiford@arm.com>]

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.guenther@gmail.com> writes:
> On Wed, Feb 22, 2023 at 12:03 AM Michael Collison <collison@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
>>