vect: Check that vector factor is a compile-time constant

  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?

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(-)

                wi::to_wide (build_int_cst (type, vf)),
  

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?
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?

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
>
  

Richard how would I check for a full masked main vector loop?

On 2/22/23 03:20, Richard Biener wrote:
> 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?
> 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?
>
> 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
>>
  

On Wed, Feb 22, 2023 at 5:42 PM Michael Collison <collison@rivosinc.com> wrote:
>
> Richard how would I check for a full masked main vector loop?

It's LOOP_VINFO_FULLY_MASKED_P I think.  For the odd prologue peeling
you see you might want to check why vect_use_loop_mask_for_alignment_p
isn't true (possibly because exactly LOOP_VINFO_FULLY_MASKED_P is not true ...)

> On 2/22/23 03:20, Richard Biener wrote:
> > 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?
> > 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?
> >
> > 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
> >>
  

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
>>
  

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.

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.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
>>>
  

On Wed, Mar 1, 2023 at 10:00 PM Michael Collison <collison@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.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
> >>>