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

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

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

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

I don't think we need to apply this limit in case of RVV auto-vectorization.
I have talked with Kito and I have a full solution of supporting RVV solution.

We are going to support RVV auto-vectorization in 3 configuration according to RVV ISA spec:
1. -march=zve32* support QI and HI auto-vectorization by VNx4QImode and VNx2HImode
2. -march=zve64* support QI and HI and SI auto-vectorization by VNx8QImode and VNx4HImode and VNx2SImode
3. -march=v* support QI and HI and SI and DI auto-vectorization by VNx16QImode and VNx8HImode and VNx4SImode and VNx2DImode

I will support them in GCC 14. Current loop vectorizer works well for us no need to fix it. 
Thanks.


juzhe.zhong@rivai.ai

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

[Michael Collison]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

Juzhe,

I disagree with this comment. There are many stakeholders for 
autovectorization and waiting until GCC 14 is not a viable solution for 
us as well as other stakeholders ready to begin work on autovectorization.

As we discussed I have been moving forward with patches for 
autovectorization and am preparing to send them to gcc-patches. This 
assert is preventing code from compiling and needs to be addressed.

If you have a solution in either the RISCV backend or in this file can 
you please present it?

On 2/22/23 10:27, juzhe.zhong@rivai.ai wrote:
> >/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)),/
>
> I don't think we need to apply this limit in case of RVV 
> auto-vectorization.
> I have talked with Kito and I have a full solution of supporting RVV 
> solution.
>
> We are going to support RVV auto-vectorization in 3 configuration 
> according to RVV ISA spec:
> 1. -march=zve32* support QI and HI auto-vectorization by VNx4QImode 
> and VNx2HImode
> 2. -march=zve64* support QI and HI and SI auto-vectorization by 
> VNx8QImode and VNx4HImode and VNx2SImode
> 3.-march=v* support QI and HI and SI and DI auto-vectorization by 
> VNx16QImode and VNx8HImode and VNx4SImode and VNx2DImode
>
> I will support them in GCC 14. Current loop vectorizer works well for 
> us no need to fix it.
> Thanks.
> ------------------------------------------------------------------------
> juzhe.zhong@rivai.ai

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

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

Currently, upstream GCC is not ready to support auto-vec.
I am building the basic infrastructure of RVV and need more testing.
I can't support auto-vec now since it depends on the infrastructure tha I am building.
I have open source "rvv-next" in RISC-V foundation repo which fully support intrinsic && auto-vec.
You can either wait for the upstream GCC or develop base rvv-next.



juzhe.zhong@rivai.ai
 
From: Michael Collison
Date: 2023-02-23 01:54
To: juzhe.zhong; gcc-patches
CC: kito.cheng; kito.cheng; richard.sandiford; richard.guenther
Subject: Re: [PATCH] vect: Check that vector factor is a compile-time constant
Juzhe,
I disagree with this comment. There are many stakeholders for autovectorization and waiting until GCC 14 is not a viable solution for us as well as other stakeholders ready to begin work on autovectorization.
As we discussed I have been moving forward with patches for autovectorization and am preparing to send them to gcc-patches. This assert is preventing code from compiling and needs to be addressed.
If you have a solution in either the RISCV backend or in this file can you please present it?
On 2/22/23 10:27, juzhe.zhong@rivai.ai wrote:
> 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)),

I don't think we need to apply this limit in case of RVV auto-vectorization.
I have talked with Kito and I have a full solution of supporting RVV solution.

We are going to support RVV auto-vectorization in 3 configuration according to RVV ISA spec:
1. -march=zve32* support QI and HI auto-vectorization by VNx4QImode and VNx2HImode
2. -march=zve64* support QI and HI and SI auto-vectorization by VNx8QImode and VNx4HImode and VNx2SImode
3. -march=v* support QI and HI and SI and DI auto-vectorization by VNx16QImode and VNx8HImode and VNx4SImode and VNx2DImode

I will support them in GCC 14. Current loop vectorizer works well for us no need to fix it. 
Thanks.


juzhe.zhong@rivai.ai

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

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 2747 bytes --]

Besides, since GCC 13 currently is on stage 4. 
Unlike the infrastructure that I am building for intrinsic && auto-vec which is safe and will not affect the original RISC-V port functionality.
Auto-vectorization will potentially affect the orignal RISC-V port functionality which is not safe to support in current stage of GCC 13.



juzhe.zhong@rivai.ai
 
From: Michael Collison
Date: 2023-02-23 01:54
To: juzhe.zhong; gcc-patches
CC: kito.cheng; kito.cheng; richard.sandiford; richard.guenther
Subject: Re: [PATCH] vect: Check that vector factor is a compile-time constant
Juzhe,
I disagree with this comment. There are many stakeholders for autovectorization and waiting until GCC 14 is not a viable solution for us as well as other stakeholders ready to begin work on autovectorization.
As we discussed I have been moving forward with patches for autovectorization and am preparing to send them to gcc-patches. This assert is preventing code from compiling and needs to be addressed.
If you have a solution in either the RISCV backend or in this file can you please present it?
On 2/22/23 10:27, juzhe.zhong@rivai.ai wrote:
> 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)),

I don't think we need to apply this limit in case of RVV auto-vectorization.
I have talked with Kito and I have a full solution of supporting RVV solution.

We are going to support RVV auto-vectorization in 3 configuration according to RVV ISA spec:
1. -march=zve32* support QI and HI auto-vectorization by VNx4QImode and VNx2HImode
2. -march=zve64* support QI and HI and SI auto-vectorization by VNx8QImode and VNx4HImode and VNx2SImode
3. -march=v* support QI and HI and SI and DI auto-vectorization by VNx16QImode and VNx8HImode and VNx4SImode and VNx2DImode

I will support them in GCC 14. Current loop vectorizer works well for us no need to fix it. 
Thanks.


juzhe.zhong@rivai.ai

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

[Jeff Law]

On 2/22/23 10:54, Michael Collison wrote:
> Juzhe,
> 
> I disagree with this comment. There are many stakeholders for 
> autovectorization and waiting until GCC 14 is not a viable solution for 
> us as well as other stakeholders ready to begin work on autovectorization.
> 
> As we discussed I have been moving forward with patches for 
> autovectorization and am preparing to send them to gcc-patches. This 
> assert is preventing code from compiling and needs to be addressed.
> 
> If you have a solution in either the RISCV backend or in this file can 
> you please present it?
I don't necessarily think it means waiting for gcc-14, but it does mean 
waiting for gcc-13 to branch and gcc-14 development to open.  I would 
object to anyone trying to push forward an autovec implementation into 
gcc-13.  We're well past that point IMHO, even if the changes only 
affected the RISC-V backend.

Given that it looks like we have two independent implementations we're 
almost certainly going to have to sit down with both, evaluate both from 
a quality of code viewpoint and benchmark them both and ultimately 
choose one implementation or the other, or maybe even some mixing and 
matching.

I would strongly suggest that both groups have implementations we can 
start evaluating from a design/implementation standpoint relatively 
soon.  Ideally both groups would actually have branches in the repo that 
are regularly updated with their current implementation.

While I have a great interest in seeing an autovec implementation move 
forward as soon as possible after gcc-14 development opens, I have no 
opinions at this point about either of the two existing implementations.

Jeff

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

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]

>> I would object to anyone trying to push forward an autovec implementation into
>> gcc-13.  We're well past that point IMHO, even if the changes only
>> affected the RISC-V backend.

Yes, I am agree with Jeff's opinion. I finished infrastructure (intrinsic and VSETVL PASS) of RVV now.
Now, I am pulling as many resources as possible to do the testing.
From now to April (until GCC 14 is open), I will only keep testing and fix bugs or some codes refine && simplification.
I won't push any more features especially autovec until GCC 14 is open.



juzhe.zhong@rivai.ai
 
From: Jeff Law
Date: 2023-02-23 12:01
To: Michael Collison; juzhe.zhong; gcc-patches
CC: kito.cheng; kito.cheng; richard.sandiford; richard.guenther
Subject: Re: [PATCH] vect: Check that vector factor is a compile-time constant
 
 
On 2/22/23 10:54, Michael Collison wrote:
> Juzhe,
> 
> I disagree with this comment. There are many stakeholders for 
> autovectorization and waiting until GCC 14 is not a viable solution for 
> us as well as other stakeholders ready to begin work on autovectorization.
> 
> As we discussed I have been moving forward with patches for 
> autovectorization and am preparing to send them to gcc-patches. This 
> assert is preventing code from compiling and needs to be addressed.
> 
> If you have a solution in either the RISCV backend or in this file can 
> you please present it?
I don't necessarily think it means waiting for gcc-14, but it does mean 
waiting for gcc-13 to branch and gcc-14 development to open.  I would 
object to anyone trying to push forward an autovec implementation into 
gcc-13.  We're well past that point IMHO, even if the changes only 
affected the RISC-V backend.
 
Given that it looks like we have two independent implementations we're 
almost certainly going to have to sit down with both, evaluate both from 
a quality of code viewpoint and benchmark them both and ultimately 
choose one implementation or the other, or maybe even some mixing and 
matching.
 
I would strongly suggest that both groups have implementations we can 
start evaluating from a design/implementation standpoint relatively 
soon.  Ideally both groups would actually have branches in the repo that 
are regularly updated with their current implementation.
 
While I have a great interest in seeing an autovec implementation move 
forward as soon as possible after gcc-14 development opens, I have no 
opinions at this point about either of the two existing implementations.
 
Jeff
 

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

[Michael Collison]

Hi Jeff,

We do not have two independent implementations: my work is 100% based on 
the vector intrinsic foundation in upstream GCC. In fact I have only 
added two core patterns, vector add and subtract, that are based on the 
existing vector intrinsics implementation:

(define_expand "add<mode>3"
   [(match_operand:VI 0 "register_operand")
    (match_operand:VI 1 "register_operand")
    (match_operand:VI 2 "vector_arith_operand")]
   "TARGET_VECTOR"
{
   using namespace riscv_vector;

   rtx merge = gen_rtx_UNSPEC (<MODE>mode, gen_rtvec (1, const0_rtx), 
UNSPEC_VUNDEF);
   rtx vl = emit_vlmax_vsetvl (<MODE>mode);
   rtx mask_policy = get_mask_policy_no_pred();
   rtx tail_policy = get_tail_policy_no_pred();
   rtx mask = CONSTM1_RTX(<VM>mode);
   rtx vlmax_avl_p = get_avl_type_rtx(NONVLMAX);

   emit_insn(gen_pred_add<mode>(operands[0], mask, merge, operands[1], 
operands[2],
                 vl, tail_policy, mask_policy, vlmax_avl_p));

   DONE;
})

This pattern leverages the existing vector intrinsics framework. The 
bulk of the changes are the cost model, and target macros. The cost 
model is based on Juzhe's work.

The point I am making is the auto-vectorization work is no more 
experimental than the intrinsics work which is still being merged.

On 2/22/23 23:01, Jeff Law wrote:
>
>
> On 2/22/23 10:54, Michael Collison wrote:
>> Juzhe,
>>
>> I disagree with this comment. There are many stakeholders for 
>> autovectorization and waiting until GCC 14 is not a viable solution 
>> for us as well as other stakeholders ready to begin work on 
>> autovectorization.
>>
>> As we discussed I have been moving forward with patches for 
>> autovectorization and am preparing to send them to gcc-patches. This 
>> assert is preventing code from compiling and needs to be addressed.
>>
>> If you have a solution in either the RISCV backend or in this file 
>> can you please present it?
> I don't necessarily think it means waiting for gcc-14, but it does 
> mean waiting for gcc-13 to branch and gcc-14 development to open. I 
> would object to anyone trying to push forward an autovec 
> implementation into gcc-13.  We're well past that point IMHO, even if 
> the changes only affected the RISC-V backend.
>
> Given that it looks like we have two independent implementations we're 
> almost certainly going to have to sit down with both, evaluate both 
> from a quality of code viewpoint and benchmark them both and 
> ultimately choose one implementation or the other, or maybe even some 
> mixing and matching.
>
> I would strongly suggest that both groups have implementations we can 
> start evaluating from a design/implementation standpoint relatively 
> soon.  Ideally both groups would actually have branches in the repo 
> that are regularly updated with their current implementation.
>
> While I have a great interest in seeing an autovec implementation move 
> forward as soon as possible after gcc-14 development opens, I have no 
> opinions at this point about either of the two existing implementations.
>
> Jeff

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

[Jeff Law]

On 2/22/23 21:50, Michael Collison wrote:
> Hi Jeff,
> 
> We do not have two independent implementations: my work is 100% based on 
> the vector intrinsic foundation in upstream GCC. 
Phew!  That's good news.  I totally misunderstood.


> In fact I have only 
> added two core patterns, vector add and subtract, that are based on the 
> existing vector intrinsics implementation:
> 
> (define_expand "add<mode>3"
>    [(match_operand:VI 0 "register_operand")
>     (match_operand:VI 1 "register_operand")
>     (match_operand:VI 2 "vector_arith_operand")]
>    "TARGET_VECTOR"
> {
>    using namespace riscv_vector;
> 
>    rtx merge = gen_rtx_UNSPEC (<MODE>mode, gen_rtvec (1, const0_rtx), 
> UNSPEC_VUNDEF);
>    rtx vl = emit_vlmax_vsetvl (<MODE>mode);
>    rtx mask_policy = get_mask_policy_no_pred();
>    rtx tail_policy = get_tail_policy_no_pred();
>    rtx mask = CONSTM1_RTX(<VM>mode);
>    rtx vlmax_avl_p = get_avl_type_rtx(NONVLMAX);
> 
>    emit_insn(gen_pred_add<mode>(operands[0], mask, merge, operands[1], 
> operands[2],
>                  vl, tail_policy, mask_policy, vlmax_avl_p));
> 
>    DONE;
> })
> 
> This pattern leverages the existing vector intrinsics framework. The 
> bulk of the changes are the cost model, and target macros. The cost 
> model is based on Juzhe's work.
Understood.

> 
> The point I am making is the auto-vectorization work is no more 
> experimental than the intrinsics work which is still being merged.
I would disagree, though I do see your point.  It's unfortunate that 
intrinsics work is still being merged -- I certainly wish that were not 
the case, but having agreed to the compromise earlier I personally feel 
that I need to let that process play out per that agreement.

--

What I'd been planning to do internally at Ventana was to update our 
codebase to gcc-13 once it's released.  Then I'd backport RVV autovec 
work from the gcc-14 dev tree into that Ventana branch.

Instead, but along the same lines, we could have a public gcc-13 based 
branch which follows that same process and where Rivos, SiFive, Rivai, 
Ventana (and potentially others with an interest in this space) could 
collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd 
probably have to hash out a bit of policy with the shared branch, but 
I'd like to think we could make it work.


Thoughts?

jeff

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

[Kito Cheng]

Hi Jeff:

> What I'd been planning to do internally at Ventana was to update our
> codebase to gcc-13 once it's released.  Then I'd backport RVV autovec
> work from the gcc-14 dev tree into that Ventana branch.
>
> Instead, but along the same lines, we could have a public gcc-13 based
> branch which follows that same process and where Rivos, SiFive, Rivai,
> Ventana (and potentially others with an interest in this space) could
> collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd
> probably have to hash out a bit of policy with the shared branch, but
> I'd like to think we could make it work.

+1, I like the idea, I could imagine we definitely will do the same
work more than four times by different companies if we don't have a
collaboration branch...

>
> Thoughts?
>
> jeff
>

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

[Jeff Law]

On 2/23/23 21:04, Kito Cheng wrote:
> Hi Jeff:
> 
>> What I'd been planning to do internally at Ventana was to update our
>> codebase to gcc-13 once it's released.  Then I'd backport RVV autovec
>> work from the gcc-14 dev tree into that Ventana branch.
>>
>> Instead, but along the same lines, we could have a public gcc-13 based
>> branch which follows that same process and where Rivos, SiFive, Rivai,
>> Ventana (and potentially others with an interest in this space) could
>> collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd
>> probably have to hash out a bit of policy with the shared branch, but
>> I'd like to think we could make it work.
> 
> +1, I like the idea, I could imagine we definitely will do the same
> work more than four times by different companies if we don't have a
> collaboration branch...
So it looks like there's a general sense that a coordination branch off 
gcc-13 is reasonable.  So I'd like to hammer out a few details.


First, I recommend we cut a branch from gcc-13 soon after gcc-13 
branches.  That way we've got a place to land the vector work.

Second, I recommend we rebase that branch periodically so that it 
follows gcc-13.  That means downstream consumers may have non-ff pulls, 
but I think we want to follow gcc-13 fairly closely.  I'm open to other 
approaches here.

Third, I was thinking that once a patch related to risc-v vectorization 
goes to the trunk, any one of the principals should be able to 
cherry-pick that patch onto our branch.

That implies we need to identify the principals.  I'll suggest Kito, 
Juzhe, Michael and myself as the initial list.  I'm certainly open to 
others joining.


Other thoughts or suggestions?

Jeff

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

[Palmer Dabbelt]

On Tue, 14 Mar 2023 10:48:24 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>
>
> On 2/23/23 21:04, Kito Cheng wrote:
>> Hi Jeff:
>>
>>> What I'd been planning to do internally at Ventana was to update our
>>> codebase to gcc-13 once it's released.  Then I'd backport RVV autovec
>>> work from the gcc-14 dev tree into that Ventana branch.
>>>
>>> Instead, but along the same lines, we could have a public gcc-13 based
>>> branch which follows that same process and where Rivos, SiFive, Rivai,
>>> Ventana (and potentially others with an interest in this space) could
>>> collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd
>>> probably have to hash out a bit of policy with the shared branch, but
>>> I'd like to think we could make it work.
>>
>> +1, I like the idea, I could imagine we definitely will do the same
>> work more than four times by different companies if we don't have a
>> collaboration branch...
> So it looks like there's a general sense that a coordination branch off
> gcc-13 is reasonable.  So I'd like to hammer out a few details.
>
>
> First, I recommend we cut a branch from gcc-13 soon after gcc-13
> branches.  That way we've got a place to land the vector work.
>
> Second, I recommend we rebase that branch periodically so that it
> follows gcc-13.  That means downstream consumers may have non-ff pulls,
> but I think we want to follow gcc-13 fairly closely.  I'm open to other
> approaches here.
>
> Third, I was thinking that once a patch related to risc-v vectorization
> goes to the trunk, any one of the principals should be able to
> cherry-pick that patch onto our branch.

I'm a little bit confused about what the proposal is here: is the idea 
to have a branch based on gcc-13 where we coordinate work before it 
lands on trunk, or a branch based on gcc-13 where we backport 
autovec-related patches once they've landed on trunk?  In my mind those 
are actually two different things and I think they're both useful, maybe 
we should just do both?

Having a shared work-in-progress branch for the autovec stuff makes 
sense to me: it's a big patch set with engineers at multiple companies 
working on it, so having a shared patch stack should help with the 
coordination.  That branch will need to get re-written as patches get 
reviewed/merged, so having it rebase seems reasonable.  I'd have the 
branch based on trunk, as that's the eventual target for the patches, 
but trunk can be unstable so maybe that'll be too much of a headache.

For pretty much every other GCC release we've ended up with a "extra 
RISC-V backports" branch, where we end up with some patches that aren't 
suitable for proper upstream backports (usually because they're a 
performance improvement).  We've always talked about doing that as a FSF 
vendor branch, but I don't think we really ever got organized enough to 
do it.  We're doing that internally anyway at Rivos and I'd bet everyone 
else is too, it'd be great to find some way to share as much of that 
work as we can.

It's sort of a headache to just propose doing everything, but in this 
case I think we're going to end up with various flavors of both of these 
branches internally at the various companies so we might as well just 
try and do that in public where we can.

> That implies we need to identify the principals.  I'll suggest Kito,
> Juzhe, Michael and myself as the initial list.  I'm certainly open to
> others joining.

+Vineet, who's been handling our internal GCC branches.

We'll still have internal branches for 13 regardless of how the autovec 
stuff proceeds, but having any sort of upstream backport branch will 
make life easier as we'll be able to share some of that work.

> Other thoughts or suggestions?

Sorry if that throws a bit of a wrench in the works.

Just for context: in Rivos land we don't have any specific timelines 
around 13, so the goal on our end is just to keep the vectorization 
stuff progressing smoothly as we spin up more engineering resources on 
it.  Our aim is just to get everything on trunk eventually, anything 
else is just a stop-gap and we can work around it (though sharing that 
work is always a win).

>
> Jeff

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

[Palmer Dabbelt]

On Tue, 14 Mar 2023 10:48:24 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>
>
> On 2/23/23 21:04, Kito Cheng wrote:
>> Hi Jeff:
>>
>>> What I'd been planning to do internally at Ventana was to update our
>>> codebase to gcc-13 once it's released.  Then I'd backport RVV autovec
>>> work from the gcc-14 dev tree into that Ventana branch.
>>>
>>> Instead, but along the same lines, we could have a public gcc-13 based
>>> branch which follows that same process and where Rivos, SiFive, Rivai,
>>> Ventana (and potentially others with an interest in this space) could
>>> collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd
>>> probably have to hash out a bit of policy with the shared branch, but
>>> I'd like to think we could make it work.
>>
>> +1, I like the idea, I could imagine we definitely will do the same
>> work more than four times by different companies if we don't have a
>> collaboration branch...
> So it looks like there's a general sense that a coordination branch off
> gcc-13 is reasonable.  So I'd like to hammer out a few details.
>
>
> First, I recommend we cut a branch from gcc-13 soon after gcc-13
> branches.  That way we've got a place to land the vector work.
>
> Second, I recommend we rebase that branch periodically so that it
> follows gcc-13.  That means downstream consumers may have non-ff pulls,
> but I think we want to follow gcc-13 fairly closely.  I'm open to other
> approaches here.
>
> Third, I was thinking that once a patch related to risc-v vectorization
> goes to the trunk, any one of the principals should be able to
> cherry-pick that patch onto our branch.

I'm a little bit confused about what the proposal is here: is the idea 
to have a branch based on gcc-13 where we coordinate work before it 
lands on trunk, or a branch based on gcc-13 where we backport 
autovec-related patches once they've landed on trunk?  In my mind those 
are actually two different things and I think they're both useful, maybe 
we should just do both?

Having a shared work-in-progress branch for the autovec stuff makes 
sense to me: it's a big patch set with engineers at multiple companies 
working on it, so having a shared patch stack should help with the 
coordination.  That branch will need to get re-written as patches get 
reviewed/merged, so having it rebase seems reasonable.  I'd have the 
branch based on trunk, as that's the eventual target for the patches, 
but trunk can be unstable so maybe that'll be too much of a headache.

For pretty much every other GCC release we've ended up with a "extra 
RISC-V backports" branch, where we end up with some patches that aren't 
suitable for proper upstream backports (usually because they're a 
performance improvement).  We've always talked about doing that as a FSF 
vendor branch, but I don't think we really ever got organized enough to 
do it.  We're doing that internally anyway at Rivos and I'd bet everyone 
else is too, it'd be great to find some way to share as much of that 
work as we can.

It's sort of a headache to just propose doing everything, but in this 
case I think we're going to end up with various flavors of both of these 
branches internally at the various companies so we might as well just 
try and do that in public where we can.

> That implies we need to identify the principals.  I'll suggest Kito,
> Juzhe, Michael and myself as the initial list.  I'm certainly open to
> others joining.

+Vineet, who's been handling our internal GCC branches.

We'll still have internal branches for 13 regardless of how the autovec 
stuff proceeds, but having any sort of upstream backport branch will 
make life easier as we'll be able to share some of that work.

> Other thoughts or suggestions?

Sorry if that throws a bit of a wrench in the works.

Just for context: in Rivos land we don't have any specific timelines 
around 13, so the goal on our end is just to keep the vectorization 
stuff progressing smoothly as we spin up more engineering resources on 
it.  Our aim is just to get everything on trunk eventually, anything 
else is just a stop-gap and we can work around it (though sharing that 
work is always a win).

>
> Jeff

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

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 5304 bytes --]

I would prefer this is the branch based on gcc-13 where we backport
autovec-related patches once they've landed on trunk.

I won't do any development on this branch, instead, I would just use it for releasing my downstream GCC.

I will directly support auto-vectorization on the trunk since the most important thing for our RVV auto-vectorization landing
in GCC is reviewing from Richard Biener && Richard sandiford.

Besides, from my experience of development RVV GCC (rvv-next) and optimization of RVV LLVM (Rivai downstream LLVM). Now, I have a bunch of new ideas
of supporting RVV auto-vectorization. I think we can have a sync up meeting (share my current new ideas) before I start to support RVV auto-vectorization before GCC 14.


juzhe.zhong@rivai.ai
 
From: Palmer Dabbelt
Date: 2023-03-18 00:57
To: gcc-patches; Vineet Gupta
CC: Kito Cheng; collison; juzhe.zhong; gcc-patches; kito.cheng; richard.sandiford; richard.guenther
Subject: Re: [PATCH] vect: Check that vector factor is a compile-time constant
On Tue, 14 Mar 2023 10:48:24 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>
>
> On 2/23/23 21:04, Kito Cheng wrote:
>> Hi Jeff:
>>
>>> What I'd been planning to do internally at Ventana was to update our
>>> codebase to gcc-13 once it's released.  Then I'd backport RVV autovec
>>> work from the gcc-14 dev tree into that Ventana branch.
>>>
>>> Instead, but along the same lines, we could have a public gcc-13 based
>>> branch which follows that same process and where Rivos, SiFive, Rivai,
>>> Ventana (and potentially others with an interest in this space) could
>>> collaborate.  Essentially it'd be gcc-13 + RVV autovec support.  We'd
>>> probably have to hash out a bit of policy with the shared branch, but
>>> I'd like to think we could make it work.
>>
>> +1, I like the idea, I could imagine we definitely will do the same
>> work more than four times by different companies if we don't have a
>> collaboration branch...
> So it looks like there's a general sense that a coordination branch off
> gcc-13 is reasonable.  So I'd like to hammer out a few details.
>
>
> First, I recommend we cut a branch from gcc-13 soon after gcc-13
> branches.  That way we've got a place to land the vector work.
>
> Second, I recommend we rebase that branch periodically so that it
> follows gcc-13.  That means downstream consumers may have non-ff pulls,
> but I think we want to follow gcc-13 fairly closely.  I'm open to other
> approaches here.
>
> Third, I was thinking that once a patch related to risc-v vectorization
> goes to the trunk, any one of the principals should be able to
> cherry-pick that patch onto our branch.
 
I'm a little bit confused about what the proposal is here: is the idea 
to have a branch based on gcc-13 where we coordinate work before it 
lands on trunk, or a branch based on gcc-13 where we backport 
autovec-related patches once they've landed on trunk?  In my mind those 
are actually two different things and I think they're both useful, maybe 
we should just do both?
 
Having a shared work-in-progress branch for the autovec stuff makes 
sense to me: it's a big patch set with engineers at multiple companies 
working on it, so having a shared patch stack should help with the 
coordination.  That branch will need to get re-written as patches get 
reviewed/merged, so having it rebase seems reasonable.  I'd have the 
branch based on trunk, as that's the eventual target for the patches, 
but trunk can be unstable so maybe that'll be too much of a headache.
 
For pretty much every other GCC release we've ended up with a "extra 
RISC-V backports" branch, where we end up with some patches that aren't 
suitable for proper upstream backports (usually because they're a 
performance improvement).  We've always talked about doing that as a FSF 
vendor branch, but I don't think we really ever got organized enough to 
do it.  We're doing that internally anyway at Rivos and I'd bet everyone 
else is too, it'd be great to find some way to share as much of that 
work as we can.
 
It's sort of a headache to just propose doing everything, but in this 
case I think we're going to end up with various flavors of both of these 
branches internally at the various companies so we might as well just 
try and do that in public where we can.
 
> That implies we need to identify the principals.  I'll suggest Kito,
> Juzhe, Michael and myself as the initial list.  I'm certainly open to
> others joining.
 
+Vineet, who's been handling our internal GCC branches.
 
We'll still have internal branches for 13 regardless of how the autovec 
stuff proceeds, but having any sort of upstream backport branch will 
make life easier as we'll be able to share some of that work.
 
> Other thoughts or suggestions?
 
Sorry if that throws a bit of a wrench in the works.
 
Just for context: in Rivos land we don't have any specific timelines 
around 13, so the goal on our end is just to keep the vectorization 
stuff progressing smoothly as we spin up more engineering resources on 
it.  Our aim is just to get everything on trunk eventually, anything 
else is just a stop-gap and we can work around it (though sharing that 
work is always a win).
 
>
> Jeff
 

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

[Jeff Law]

On 3/17/23 10:57, Palmer Dabbelt wrote:

> 
> I'm a little bit confused about what the proposal is here: is the idea 
> to have a branch based on gcc-13 where we coordinate work before it 
> lands on trunk, or a branch based on gcc-13 where we backport 
> autovec-related patches once they've landed on trunk?  In my mind those 
> are actually two different things and I think they're both useful, maybe 
> we should just do both?
I was thinking it was a branch to coordinate backports.  We could also 
have a branch to coordinate development before it lands on the trunk.


The former provides a base for those who might want a stable gcc-13 
based compiler, but with RVV support.  The latter is more focused on 
ongoing development.


> 
>> That implies we need to identify the principals.  I'll suggest Kito,
>> Juzhe, Michael and myself as the initial list.  I'm certainly open to
>> others joining.
> 
> +Vineet, who's been handling our internal GCC branches.
OK.


> 
> Sorry if that throws a bit of a wrench in the works.
No worries at all.

> 
> Just for context: in Rivos land we don't have any specific timelines 
> around 13, so the goal on our end is just to keep the vectorization 
> stuff progressing smoothly as we spin up more engineering resources on 
> it.  Our aim is just to get everything on trunk eventually, anything 
> else is just a stop-gap and we can work around it (though sharing that 
> work is always a win).
We don't have hard time lines (yet), but I can work backwards from 
various plans and conclude that Ventana will need a gcc-13 with vector 
backports, hence my original focus on that aspect of the coordination 
problem.

Thanks for raising the need for a development coordination branch.

jeff

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

[Palmer Dabbelt]

On Thu, 23 Mar 2023 16:18:20 PDT (-0700), jeffreyalaw@gmail.com wrote:
>
>
> On 3/17/23 10:57, Palmer Dabbelt wrote:
>
>>
>> I'm a little bit confused about what the proposal is here: is the idea
>> to have a branch based on gcc-13 where we coordinate work before it
>> lands on trunk, or a branch based on gcc-13 where we backport
>> autovec-related patches once they've landed on trunk?  In my mind those
>> are actually two different things and I think they're both useful, maybe
>> we should just do both?
> I was thinking it was a branch to coordinate backports.  We could also
> have a branch to coordinate development before it lands on the trunk.
>
>
> The former provides a base for those who might want a stable gcc-13
> based compiler, but with RVV support.  The latter is more focused on
> ongoing development.

Yep, just two different things.

>>> That implies we need to identify the principals.  I'll suggest Kito,
>>> Juzhe, Michael and myself as the initial list.  I'm certainly open to
>>> others joining.
>>
>> +Vineet, who's been handling our internal GCC branches.
> OK.
>
>
>>
>> Sorry if that throws a bit of a wrench in the works.
> No worries at all.
>
>>
>> Just for context: in Rivos land we don't have any specific timelines
>> around 13, so the goal on our end is just to keep the vectorization
>> stuff progressing smoothly as we spin up more engineering resources on
>> it.  Our aim is just to get everything on trunk eventually, anything
>> else is just a stop-gap and we can work around it (though sharing that
>> work is always a win).
> We don't have hard time lines (yet), but I can work backwards from
> various plans and conclude that Ventana will need a gcc-13 with vector
> backports, hence my original focus on that aspect of the coordination
> problem.

OK.  We don't have a hard need there, but it'll make life easier so I'm 
happy to just treat it like a real shipping branch if you guys are going 
to as well.

Are you OK just having a single "gcc-13 with RISC-V performance 
backports" branch, or do you want just vector backports?  Our internal 
branch would be all performance-related backports, but no big deal if 
the upstream stuff is vector-only as that's probably going to be 90%+ of 
the churn.

> Thanks for raising the need for a development coordination branch.

I guess "need" is kind of strong: IMO it's up to the people actually 
doing the work how to organize the branches.  I'm not writing the code 
here so I'm happy with whatever, just pointing out that there's two 
different things that could be done ;)

> jeff

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

[Jeff Law]

On 3/23/23 20:28, Palmer Dabbelt wrote:
> On Thu, 23 Mar 2023 16:18:20 PDT (-0700), jeffreyalaw@gmail.com wrote:

> 
> OK.  We don't have a hard need there, but it'll make life easier so I'm 
> happy to just treat it like a real shipping branch if you guys are going 
> to as well.
I'd planned to use it for Ventana's gcc-13 baseline since we're going to 
want RVV support before gcc-14 hits the streets.  So from Ventana's 
viewpoint is's like a real shipping branch.

> 
> Are you OK just having a single "gcc-13 with RISC-V performance 
> backports" branch, or do you want just vector backports?  Our internal 
> branch would be all performance-related backports, but no big deal if 
> the upstream stuff is vector-only as that's probably going to be 90%+ of 
> the churn.
I can live with performance backports.  It wasn't my original intent, 
but I see the benefits to the ecosystem.

> 
>> Thanks for raising the need for a development coordination branch.
> 
> I guess "need" is kind of strong: IMO it's up to the people actually 
> doing the work how to organize the branches.  I'm not writing the code 
> here so I'm happy with whatever, just pointing out that there's two 
> different things that could be done ;)
I think there's enough interested parties for the development side as 
well that ca;ling it a "need" isn't a significant stretch.

jeff

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

[Michael Collison]

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

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

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

[Richard Biener]

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
>

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

[Michael Collison]

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

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

[Richard Biener]

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

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

[Richard Sandiford]

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

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

[Michael Collison]

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

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

[Richard Biener]

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