* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-30 12:33 ` Richard Biener
@ 2023-05-30 12:37 ` 钟居哲
[not found] ` <FA43CAC5-BCCE-42AF-8A6B-E69F1A496F5C@suse.de>
2023-05-30 14:13 ` 钟居哲
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: 钟居哲 @ 2023-05-30 12:37 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 2758 bytes --]
>> That's odd, you only need to adjust the IV which is used in the exit test,
>> not all the others.
Sorry for my incorrect information. I checked the codegen of both single-rgroup and multi-rgroup.
Their codegen are same behavior, after this patch, there will be 1 more neg instruction in preheader
and 1 more mv instruction inside the loop.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-30 20:33
To: juzhe.zhong
CC: Richard Sandiford; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Tue, 30 May 2023, juzhe.zhong wrote:
> This patch will generate the number of rgroup ?mov? instructions inside the
> loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> instruction in loop. If this patch is necessary? I think I should find a way
> to fix it.
That's odd, you only need to adjust the IV which is used in the exit test,
not all the others.
> ---- Replied Message ----
> From
> Richard Sandiford<richard.sandiford@arm.com>
> Date
> 05/30/2023 19:41
> To
> juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> Cc
> gcc-patches<gcc-patches@gcc.gnu.org>,
> rguenther<rguenther@suse.de>,
> linkw<linkw@linux.ibm.com>
> Subject
> Re: [PATCH] VECT: Change flow of decrement IV
> "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > Before this patch:
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > sub a2,a2,a5
> > bne a2,zero,.L3
> > .L5:
> > ret
> >
> > After this patch:
> >
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > neg a7,a4 -->>>additional instruction
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > mv a6,a2 -->>>additional instruction
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > add a2,a2,a7
> > bgtu a6,a4,.L3
> > .L5:
> > ret
> >
> > There is 1 more instruction in preheader and 1 more instruction in loop.
> > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> this issue will gone.
>
> But what about cases where you won't be using SELECT_VL, such as SLP?
>
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-30 12:33 ` Richard Biener
2023-05-30 12:37 ` 钟居哲
@ 2023-05-30 14:13 ` 钟居哲
2023-05-30 14:47 ` 钟居哲
2023-05-30 15:05 ` 钟居哲
3 siblings, 0 replies; 26+ messages in thread
From: 钟居哲 @ 2023-05-30 14:13 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 4748 bytes --]
Hi, all. After several investigations:
Here is my experiements:
void
single_rgroup (int32_t *__restrict a, int32_t *__restrict b, int n)
{
for (int i = 0; i < n; i++)
a[i] = b[i] + a[i];
}
void
mutiple_rgroup (float *__restrict f, double *__restrict d, int n)
{
for (int i = 0; i < n; ++i)
{
f[i * 2 + 0] = 1;
f[i * 2 + 1] = 2;
d[i] = 3;
}
}
single_rgroup:
ble a2,zero,.L5
li a4,4
.L3:
minu a5,a2,a4
vsetvli zero,a5,e32,m1,ta,ma
vle32.v v1,0(a0)
vle32.v v2,0(a1)
vsetivli zero,4,e32,m1,ta,ma
mv a3,a2 ---------> 1 more "mv" instruction
vadd.vv v1,v1,v2
vsetvli zero,a5,e32,m1,ta,ma
vse32.v v1,0(a0)
addi a1,a1,16
addi a0,a0,16
addi a2,a2,-4
bgtu a3,a4,.L3
.L5:
ret
.size single_rgroup, .-single_rgroup
.align 1
.globl foo5
.type foo5, @function
mutiple_rgroup :
ble a2,zero,.L11
lui a5,%hi(.LANCHOR0)
addi a5,a5,%lo(.LANCHOR0)
vl1re32.v v2,0(a5)
lui a5,%hi(.LANCHOR0+16)
addi a5,a5,%lo(.LANCHOR0+16)
slli a2,a2,1
li a3,8
li a7,4
vl1re64.v v1,0(a5)
.L9:
minu a5,a2,a3
minu a4,a5,a7
sub a5,a5,a4
addi a6,a0,16
vsetvli zero,a4,e32,m1,ta,ma
vse32.v v2,0(a0)
srli a4,a4,1
vsetvli zero,a5,e32,m1,ta,ma
vse32.v v2,0(a6)
srli a5,a5,1
vsetvli zero,a4,e64,m1,ta,ma
addi a6,a1,16
vse64.v v1,0(a1)
mv a4,a2 ---------> 1 more "mv" instruction
vsetvli zero,a5,e64,m1,ta,ma
vse64.v v1,0(a6)
addi a0,a0,32
addi a1,a1,32
addi a2,a2,-8
bgtu a4,a3,.L9
.L11:
ret
These are the examples, I have tried enough amount cases. This is the worst case after this patch for RVV:
no matter single-rgroup or multiple-rgroup, we will end up with 1 more "mv" instruction inside the loop.
There are also some examples I have tried with no more instructions (It seems IVOPTS has done some optimization in some cases).
From my side (RVV), I think one more "mv" instruction is not a big deal if this patch (apply vf step and check conditon by remain > vf)
can help IBM.
For single-rgroup, this 'mv' instruction will gone when we use SELECT_VL. For multiple-rgroup, the 'mv' instruction remains
but as I said, not a big deal.
If this patch's approach is approved, I will rebase and send SELECT_VL patch again base on this patch.
Looking forward your suggestions.
Thanks.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-30 20:33
To: juzhe.zhong
CC: Richard Sandiford; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Tue, 30 May 2023, juzhe.zhong wrote:
> This patch will generate the number of rgroup ?mov? instructions inside the
> loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> instruction in loop. If this patch is necessary? I think I should find a way
> to fix it.
That's odd, you only need to adjust the IV which is used in the exit test,
not all the others.
> ---- Replied Message ----
> From
> Richard Sandiford<richard.sandiford@arm.com>
> Date
> 05/30/2023 19:41
> To
> juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> Cc
> gcc-patches<gcc-patches@gcc.gnu.org>,
> rguenther<rguenther@suse.de>,
> linkw<linkw@linux.ibm.com>
> Subject
> Re: [PATCH] VECT: Change flow of decrement IV
> "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > Before this patch:
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > sub a2,a2,a5
> > bne a2,zero,.L3
> > .L5:
> > ret
> >
> > After this patch:
> >
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > neg a7,a4 -->>>additional instruction
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > mv a6,a2 -->>>additional instruction
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > add a2,a2,a7
> > bgtu a6,a4,.L3
> > .L5:
> > ret
> >
> > There is 1 more instruction in preheader and 1 more instruction in loop.
> > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> this issue will gone.
>
> But what about cases where you won't be using SELECT_VL, such as SLP?
>
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-30 12:33 ` Richard Biener
2023-05-30 12:37 ` 钟居哲
2023-05-30 14:13 ` 钟居哲
@ 2023-05-30 14:47 ` 钟居哲
2023-05-30 15:05 ` 钟居哲
3 siblings, 0 replies; 26+ messages in thread
From: 钟居哲 @ 2023-05-30 14:47 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 6256 bytes --]
Also, I have investigated power's testcase in RVV:
#include <stdint.h>
#define TEST_ALL(T) \
T (int8_t) \
T (uint8_t) \
T (int16_t) \
T (uint16_t) \
T (int32_t) \
T (uint32_t) \
T (int64_t) \
T (uint64_t) \
T (float) \
T (double)
#define N 64
#define START 1
#define END 59
#define test(TYPE) \
TYPE x_##TYPE[N] __attribute__((aligned(16))); \
void __attribute__((noinline, noclone)) test_npeel_##TYPE() { \
TYPE v = 0; \
for (unsigned int i = START; i < END; i++) { \
x_##TYPE[i] = v; \
v += 1; \
} \
}
TEST_ALL (test)
RVV compile option:
-march=rv64gcv_zba_zbb_zbc_zbs_zvl256b -O2 -ftree-vectorize -fno-vect-cost-model -fno-unroll-loops -ffast-math --param=riscv-autovec-preference=fixed-vlmax -S -fdump-tree-optimized
Before this patch:
void test_npeel_int16_t ()
{
unsigned long ivtmp.39;
vector(16) short int vect_vec_iv_.33;
void * _2;
vector(16) short int * _8;
vector(16) short int _10;
unsigned long loop_len_19;
unsigned long ivtmp_21;
unsigned long ivtmp_22;
<bb 2> [local count: 18146240]:
ivtmp.39_13 = (unsigned long) &MEM <int16_t[64]> [(void *)&x_int16_t + 2B];
<bb 3> [local count: 72584963]:
# vect_vec_iv_.33_12 = PHI <_10(3), { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 }(2)>
# ivtmp_21 = PHI <ivtmp_22(3), 58(2)>
# ivtmp.39_5 = PHI <ivtmp.39_14(3), ivtmp.39_13(2)>
loop_len_19 = MIN_EXPR <ivtmp_21, 16>;
_10 = vect_vec_iv_.33_12 + { 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16, 16 };
_2 = (void *) ivtmp.39_5;
_8 = &MEM <vector(16) short int> [(short int *)_2];
.LEN_STORE (_8, 16B, loop_len_19, vect_vec_iv_.33_12, 0);
ivtmp_22 = ivtmp_21 - loop_len_19;
ivtmp.39_14 = ivtmp.39_5 + 32;
if (ivtmp_22 != 0)
goto <bb 3>; [75.00%]
else
goto <bb 4>; [25.00%]
<bb 4> [local count: 18146240]:
return;
}
After this patch:
void test_npeel_int16_t ()
{
<bb 2> [local count: 18146240]:
.LEN_STORE (&MEM <int16_t[64]> [(void *)&x_int16_t + 2B], 16B, 32, { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }, 0);
.LEN_STORE (&MEM <int16_t[64]> [(void *)&x_int16_t + 66B], 16B, 26, { 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63 }, 0); [tail call]
return;
}
It seems this patch fixed power's issue now.
So, My conclusion:
1. This patch does produce 1 more redundant 'mv' instructions in some cases (not all cases). But it can partially be solved by select_vl
pattern. And even we can't fix this issue, one more 'mv' instruction is not a big deal for RVV.
2. This patch can solve power's issue.
Thanks.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-30 20:33
To: juzhe.zhong
CC: Richard Sandiford; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Tue, 30 May 2023, juzhe.zhong wrote:
> This patch will generate the number of rgroup ?mov? instructions inside the
> loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> instruction in loop. If this patch is necessary? I think I should find a way
> to fix it.
That's odd, you only need to adjust the IV which is used in the exit test,
not all the others.
> ---- Replied Message ----
> From
> Richard Sandiford<richard.sandiford@arm.com>
> Date
> 05/30/2023 19:41
> To
> juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> Cc
> gcc-patches<gcc-patches@gcc.gnu.org>,
> rguenther<rguenther@suse.de>,
> linkw<linkw@linux.ibm.com>
> Subject
> Re: [PATCH] VECT: Change flow of decrement IV
> "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > Before this patch:
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > sub a2,a2,a5
> > bne a2,zero,.L3
> > .L5:
> > ret
> >
> > After this patch:
> >
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > neg a7,a4 -->>>additional instruction
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > mv a6,a2 -->>>additional instruction
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > add a2,a2,a7
> > bgtu a6,a4,.L3
> > .L5:
> > ret
> >
> > There is 1 more instruction in preheader and 1 more instruction in loop.
> > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> this issue will gone.
>
> But what about cases where you won't be using SELECT_VL, such as SLP?
>
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-30 12:33 ` Richard Biener
` (2 preceding siblings ...)
2023-05-30 14:47 ` 钟居哲
@ 2023-05-30 15:05 ` 钟居哲
2023-05-31 1:42 ` juzhe.zhong
3 siblings, 1 reply; 26+ messages in thread
From: 钟居哲 @ 2023-05-30 15:05 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]
More information of power's testcase:
Before this patch:
test_npeel_int16_t:
lui a4,%hi(.LANCHOR0+130)
lui a3,%hi(.LANCHOR1)
addi a3,a3,%lo(.LANCHOR1)
addi a4,a4,%lo(.LANCHOR0+130)
li a5,58
li a2,16
vsetivli zero,16,e16,m1,ta,ma
vl1re16.v v3,0(a3)
vid.v v1
.L5:
minu a3,a5,a2
vsetvli zero,a3,e16,m1,ta,ma
sub a5,a5,a3
vse16.v v1,0(a4)
vsetivli zero,16,e16,m1,ta,ma
addi a4,a4,32
vadd.vv v1,v1,v3
bne a5,zero,.L5
ret
After this patch:
test_npeel_int16_t:
lui a5,%hi(.LANCHOR0)
addi a5,a5,%lo(.LANCHOR0)
li a1,16
vsetivli zero,16,e16,m1,ta,ma
addi a2,a5,130
vid.v v1
addi a3,a5,162
vadd.vx v4,v1,a1
addi a4,a5,194
li a1,32
vadd.vx v3,v1,a1
vse16.v v1,0(a2)
vse16.v v4,0(a3)
vse16.v v3,0(a4)
addi a5,a5,226
li a1,48
vadd.vx v2,v1,a1
vsetivli zero,10,e16,m1,ta,ma
vse16.v v2,0(a5)
ret
It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-30 20:33
To: juzhe.zhong
CC: Richard Sandiford; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Tue, 30 May 2023, juzhe.zhong wrote:
> This patch will generate the number of rgroup ?mov? instructions inside the
> loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> instruction in loop. If this patch is necessary? I think I should find a way
> to fix it.
That's odd, you only need to adjust the IV which is used in the exit test,
not all the others.
> ---- Replied Message ----
> From
> Richard Sandiford<richard.sandiford@arm.com>
> Date
> 05/30/2023 19:41
> To
> juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> Cc
> gcc-patches<gcc-patches@gcc.gnu.org>,
> rguenther<rguenther@suse.de>,
> linkw<linkw@linux.ibm.com>
> Subject
> Re: [PATCH] VECT: Change flow of decrement IV
> "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > Before this patch:
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > sub a2,a2,a5
> > bne a2,zero,.L3
> > .L5:
> > ret
> >
> > After this patch:
> >
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > neg a7,a4 -->>>additional instruction
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > mv a6,a2 -->>>additional instruction
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > add a2,a2,a7
> > bgtu a6,a4,.L3
> > .L5:
> > ret
> >
> > There is 1 more instruction in preheader and 1 more instruction in loop.
> > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> this issue will gone.
>
> But what about cases where you won't be using SELECT_VL, such as SLP?
>
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-30 15:05 ` 钟居哲
@ 2023-05-31 1:42 ` juzhe.zhong
2023-05-31 6:41 ` Richard Biener
0 siblings, 1 reply; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 1:42 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 4492 bytes --]
Hi，all. I have posted my several investigations:
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
Turns out when "niters is a constant value and vf is a constant value"
This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
Actually, for current flow:
step = MIN ()
...
remain = remain - step.
I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
So, could you make a decision for this patch?
I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
we should extend SCEV/IVOPTS ?
Thanks.
juzhe.zhong@rivai.ai
From: 钟居哲
Date: 2023-05-30 23:05
To: rguenther
CC: richard.sandiford; gcc-patches; linkw
Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
More information of power's testcase:
Before this patch:
test_npeel_int16_t:
lui a4,%hi(.LANCHOR0+130)
lui a3,%hi(.LANCHOR1)
addi a3,a3,%lo(.LANCHOR1)
addi a4,a4,%lo(.LANCHOR0+130)
li a5,58
li a2,16
vsetivli zero,16,e16,m1,ta,ma
vl1re16.v v3,0(a3)
vid.v v1
.L5:
minu a3,a5,a2
vsetvli zero,a3,e16,m1,ta,ma
sub a5,a5,a3
vse16.v v1,0(a4)
vsetivli zero,16,e16,m1,ta,ma
addi a4,a4,32
vadd.vv v1,v1,v3
bne a5,zero,.L5
ret
After this patch:
test_npeel_int16_t:
lui a5,%hi(.LANCHOR0)
addi a5,a5,%lo(.LANCHOR0)
li a1,16
vsetivli zero,16,e16,m1,ta,ma
addi a2,a5,130
vid.v v1
addi a3,a5,162
vadd.vx v4,v1,a1
addi a4,a5,194
li a1,32
vadd.vx v3,v1,a1
vse16.v v1,0(a2)
vse16.v v4,0(a3)
vse16.v v3,0(a4)
addi a5,a5,226
li a1,48
vadd.vx v2,v1,a1
vsetivli zero,10,e16,m1,ta,ma
vse16.v v2,0(a5)
ret
It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-30 20:33
To: juzhe.zhong
CC: Richard Sandiford; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Tue, 30 May 2023, juzhe.zhong wrote:
> This patch will generate the number of rgroup ?mov? instructions inside the
> loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> instruction in loop. If this patch is necessary? I think I should find a way
> to fix it.
That's odd, you only need to adjust the IV which is used in the exit test,
not all the others.
> ---- Replied Message ----
> From
> Richard Sandiford<richard.sandiford@arm.com>
> Date
> 05/30/2023 19:41
> To
> juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> Cc
> gcc-patches<gcc-patches@gcc.gnu.org>,
> rguenther<rguenther@suse.de>,
> linkw<linkw@linux.ibm.com>
> Subject
> Re: [PATCH] VECT: Change flow of decrement IV
> "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > Before this patch:
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > sub a2,a2,a5
> > bne a2,zero,.L3
> > .L5:
> > ret
> >
> > After this patch:
> >
> > foo:
> > ble a2,zero,.L5
> > csrr a3,vlenb
> > srli a4,a3,2
> > neg a7,a4 -->>>additional instruction
> > .L3:
> > minu a5,a2,a4
> > vsetvli zero,a5,e32,m1,ta,ma
> > vle32.v v2,0(a1)
> > vle32.v v1,0(a0)
> > vsetvli t1,zero,e32,m1,ta,ma
> > mv a6,a2 -->>>additional instruction
> > vadd.vv v1,v1,v2
> > vsetvli zero,a5,e32,m1,ta,ma
> > vse32.v v1,0(a0)
> > add a1,a1,a3
> > add a0,a0,a3
> > add a2,a2,a7
> > bgtu a6,a4,.L3
> > .L5:
> > ret
> >
> > There is 1 more instruction in preheader and 1 more instruction in loop.
> > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> this issue will gone.
>
> But what about cases where you won't be using SELECT_VL, such as SLP?
>
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 1:42 ` juzhe.zhong
@ 2023-05-31 6:41 ` Richard Biener
2023-05-31 6:50 ` juzhe.zhong
2023-05-31 7:28 ` Richard Sandiford
0 siblings, 2 replies; 26+ messages in thread
From: Richard Biener @ 2023-05-31 6:41 UTC (permalink / raw)
To: juzhe.zhong; +Cc: richard.sandiford, gcc-patches, linkw
On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> Hi?all. I have posted my several investigations:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>
> Turns out when "niters is a constant value and vf is a constant value"
> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>
> Actually, for current flow:
>
> step = MIN ()
> ...
> remain = remain - step.
>
> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> So, could you make a decision for this patch?
>
> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> we should extend SCEV/IVOPTS ?
I don't think we can do anything in SCEV for this which means we'd
need to special-case this in niter analysis, in IVOPTs and any other
passes that might be affected (and not fixed by handling it in niter
analysis). While improving niter analysis would be good (the user
could write this pattern as well) I do not have time to try
implementing that (I have no idea how ugly or robust it is going to be).
So I think we should patch this up in the vectorizer itself like with
your patch. I'm going to wait for Richards input though since he
seems to disagree.
Note with SELECT_VL all bets will be off since as I understand the
value it gives can vary from iteration to iteration (but we know
a lower and maybe an upper bound?)
Thanks,
Richard.
> Thanks.
>
>
> juzhe.zhong@rivai.ai
>
> From: ???
> Date: 2023-05-30 23:05
> To: rguenther
> CC: richard.sandiford; gcc-patches; linkw
> Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
> More information of power's testcase:
>
> Before this patch:
> test_npeel_int16_t:
> lui a4,%hi(.LANCHOR0+130)
> lui a3,%hi(.LANCHOR1)
> addi a3,a3,%lo(.LANCHOR1)
> addi a4,a4,%lo(.LANCHOR0+130)
> li a5,58
> li a2,16
> vsetivli zero,16,e16,m1,ta,ma
> vl1re16.v v3,0(a3)
> vid.v v1
> .L5:
> minu a3,a5,a2
> vsetvli zero,a3,e16,m1,ta,ma
> sub a5,a5,a3
> vse16.v v1,0(a4)
> vsetivli zero,16,e16,m1,ta,ma
> addi a4,a4,32
> vadd.vv v1,v1,v3
> bne a5,zero,.L5
> ret
>
> After this patch:
> test_npeel_int16_t:
> lui a5,%hi(.LANCHOR0)
> addi a5,a5,%lo(.LANCHOR0)
> li a1,16
> vsetivli zero,16,e16,m1,ta,ma
> addi a2,a5,130
> vid.v v1
> addi a3,a5,162
> vadd.vx v4,v1,a1
> addi a4,a5,194
> li a1,32
> vadd.vx v3,v1,a1
> vse16.v v1,0(a2)
> vse16.v v4,0(a3)
> vse16.v v3,0(a4)
> addi a5,a5,226
> li a1,48
> vadd.vx v2,v1,a1
> vsetivli zero,10,e16,m1,ta,ma
> vse16.v v2,0(a5)
> ret
>
> It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
>
>
> juzhe.zhong@rivai.ai
>
> From: Richard Biener
> Date: 2023-05-30 20:33
> To: juzhe.zhong
> CC: Richard Sandiford; gcc-patches; linkw
> Subject: Re: [PATCH] VECT: Change flow of decrement IV
> On Tue, 30 May 2023, juzhe.zhong wrote:
>
> > This patch will generate the number of rgroup ?mov? instructions inside the
> > loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> > instruction in loop. If this patch is necessary? I think I should find a way
> > to fix it.
>
> That's odd, you only need to adjust the IV which is used in the exit test,
> not all the others.
>
> > ---- Replied Message ----
> > From
> > Richard Sandiford<richard.sandiford@arm.com>
> > Date
> > 05/30/2023 19:41
> > To
> > juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> > Cc
> > gcc-patches<gcc-patches@gcc.gnu.org>,
> > rguenther<rguenther@suse.de>,
> > linkw<linkw@linux.ibm.com>
> > Subject
> > Re: [PATCH] VECT: Change flow of decrement IV
> > "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > > Before this patch:
> > > foo:
> > > ble a2,zero,.L5
> > > csrr a3,vlenb
> > > srli a4,a3,2
> > > .L3:
> > > minu a5,a2,a4
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vle32.v v2,0(a1)
> > > vle32.v v1,0(a0)
> > > vsetvli t1,zero,e32,m1,ta,ma
> > > vadd.vv v1,v1,v2
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vse32.v v1,0(a0)
> > > add a1,a1,a3
> > > add a0,a0,a3
> > > sub a2,a2,a5
> > > bne a2,zero,.L3
> > > .L5:
> > > ret
> > >
> > > After this patch:
> > >
> > > foo:
> > > ble a2,zero,.L5
> > > csrr a3,vlenb
> > > srli a4,a3,2
> > > neg a7,a4 -->>>additional instruction
> > > .L3:
> > > minu a5,a2,a4
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vle32.v v2,0(a1)
> > > vle32.v v1,0(a0)
> > > vsetvli t1,zero,e32,m1,ta,ma
> > > mv a6,a2 -->>>additional instruction
> > > vadd.vv v1,v1,v2
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vse32.v v1,0(a0)
> > > add a1,a1,a3
> > > add a0,a0,a3
> > > add a2,a2,a7
> > > bgtu a6,a4,.L3
> > > .L5:
> > > ret
> > >
> > > There is 1 more instruction in preheader and 1 more instruction in loop.
> > > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> > this issue will gone.
> >
> > But what about cases where you won't be using SELECT_VL, such as SLP?
> >
> > Richard
> >
> >
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 6:41 ` Richard Biener
@ 2023-05-31 6:50 ` juzhe.zhong
2023-05-31 7:38 ` Kewen.Lin
2023-05-31 7:28 ` Richard Sandiford
1 sibling, 1 reply; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 6:50 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 6576 bytes --]
Hi, Richi.
>> Note with SELECT_VL all bets will be off since as I understand the
>> value it gives can vary from iteration to iteration (but we know
>> a lower and maybe an upper bound?)
Yes, in RVV side, the SELECT_VL output can be in range of [ceil(avl/2), vlmax],
can be any value between the range depending on the hardware implementation.
>> So I think we should patch this up in the vectorizer itself like with
>> your patch. I'm going to wait for Richards input though since he
>> seems to disagree.
According to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109971,
Kewen is happy with this patch, turns out this patch can fix power's issue.
So, Let's wait for Richard's comments.
Thanks.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-31 14:41
To: juzhe.zhong@rivai.ai
CC: richard.sandiford; gcc-patches; linkw
Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> Hi?all. I have posted my several investigations:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>
> Turns out when "niters is a constant value and vf is a constant value"
> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>
> Actually, for current flow:
>
> step = MIN ()
> ...
> remain = remain - step.
>
> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> So, could you make a decision for this patch?
>
> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> we should extend SCEV/IVOPTS ?
I don't think we can do anything in SCEV for this which means we'd
need to special-case this in niter analysis, in IVOPTs and any other
passes that might be affected (and not fixed by handling it in niter
analysis). While improving niter analysis would be good (the user
could write this pattern as well) I do not have time to try
implementing that (I have no idea how ugly or robust it is going to be).
So I think we should patch this up in the vectorizer itself like with
your patch. I'm going to wait for Richards input though since he
seems to disagree.
Note with SELECT_VL all bets will be off since as I understand the
value it gives can vary from iteration to iteration (but we know
a lower and maybe an upper bound?)
Thanks,
Richard.
> Thanks.
>
>
> juzhe.zhong@rivai.ai
>
> From: ???
> Date: 2023-05-30 23:05
> To: rguenther
> CC: richard.sandiford; gcc-patches; linkw
> Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
> More information of power's testcase:
>
> Before this patch:
> test_npeel_int16_t:
> lui a4,%hi(.LANCHOR0+130)
> lui a3,%hi(.LANCHOR1)
> addi a3,a3,%lo(.LANCHOR1)
> addi a4,a4,%lo(.LANCHOR0+130)
> li a5,58
> li a2,16
> vsetivli zero,16,e16,m1,ta,ma
> vl1re16.v v3,0(a3)
> vid.v v1
> .L5:
> minu a3,a5,a2
> vsetvli zero,a3,e16,m1,ta,ma
> sub a5,a5,a3
> vse16.v v1,0(a4)
> vsetivli zero,16,e16,m1,ta,ma
> addi a4,a4,32
> vadd.vv v1,v1,v3
> bne a5,zero,.L5
> ret
>
> After this patch:
> test_npeel_int16_t:
> lui a5,%hi(.LANCHOR0)
> addi a5,a5,%lo(.LANCHOR0)
> li a1,16
> vsetivli zero,16,e16,m1,ta,ma
> addi a2,a5,130
> vid.v v1
> addi a3,a5,162
> vadd.vx v4,v1,a1
> addi a4,a5,194
> li a1,32
> vadd.vx v3,v1,a1
> vse16.v v1,0(a2)
> vse16.v v4,0(a3)
> vse16.v v3,0(a4)
> addi a5,a5,226
> li a1,48
> vadd.vx v2,v1,a1
> vsetivli zero,10,e16,m1,ta,ma
> vse16.v v2,0(a5)
> ret
>
> It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
>
>
> juzhe.zhong@rivai.ai
>
> From: Richard Biener
> Date: 2023-05-30 20:33
> To: juzhe.zhong
> CC: Richard Sandiford; gcc-patches; linkw
> Subject: Re: [PATCH] VECT: Change flow of decrement IV
> On Tue, 30 May 2023, juzhe.zhong wrote:
>
> > This patch will generate the number of rgroup ?mov? instructions inside the
> > loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> > instruction in loop. If this patch is necessary? I think I should find a way
> > to fix it.
>
> That's odd, you only need to adjust the IV which is used in the exit test,
> not all the others.
>
> > ---- Replied Message ----
> > From
> > Richard Sandiford<richard.sandiford@arm.com>
> > Date
> > 05/30/2023 19:41
> > To
> > juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> > Cc
> > gcc-patches<gcc-patches@gcc.gnu.org>,
> > rguenther<rguenther@suse.de>,
> > linkw<linkw@linux.ibm.com>
> > Subject
> > Re: [PATCH] VECT: Change flow of decrement IV
> > "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > > Before this patch:
> > > foo:
> > > ble a2,zero,.L5
> > > csrr a3,vlenb
> > > srli a4,a3,2
> > > .L3:
> > > minu a5,a2,a4
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vle32.v v2,0(a1)
> > > vle32.v v1,0(a0)
> > > vsetvli t1,zero,e32,m1,ta,ma
> > > vadd.vv v1,v1,v2
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vse32.v v1,0(a0)
> > > add a1,a1,a3
> > > add a0,a0,a3
> > > sub a2,a2,a5
> > > bne a2,zero,.L3
> > > .L5:
> > > ret
> > >
> > > After this patch:
> > >
> > > foo:
> > > ble a2,zero,.L5
> > > csrr a3,vlenb
> > > srli a4,a3,2
> > > neg a7,a4 -->>>additional instruction
> > > .L3:
> > > minu a5,a2,a4
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vle32.v v2,0(a1)
> > > vle32.v v1,0(a0)
> > > vsetvli t1,zero,e32,m1,ta,ma
> > > mv a6,a2 -->>>additional instruction
> > > vadd.vv v1,v1,v2
> > > vsetvli zero,a5,e32,m1,ta,ma
> > > vse32.v v1,0(a0)
> > > add a1,a1,a3
> > > add a0,a0,a3
> > > add a2,a2,a7
> > > bgtu a6,a4,.L3
> > > .L5:
> > > ret
> > >
> > > There is 1 more instruction in preheader and 1 more instruction in loop.
> > > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> > this issue will gone.
> >
> > But what about cases where you won't be using SELECT_VL, such as SLP?
> >
> > Richard
> >
> >
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 6:50 ` juzhe.zhong
@ 2023-05-31 7:38 ` Kewen.Lin
2023-05-31 7:50 ` juzhe.zhong
0 siblings, 1 reply; 26+ messages in thread
From: Kewen.Lin @ 2023-05-31 7:38 UTC (permalink / raw)
To: juzhe.zhong; +Cc: richard.sandiford, gcc-patches, rguenther
> Hi, Richi.
>
>>> Note with SELECT_VL all bets will be off since as I understand the
>>> value it gives can vary from iteration to iteration (but we know
>>> a lower and maybe an upper bound?)
> Yes, in RVV side, the SELECT_VL output can be in range of [ceil(avl/2), vlmax],
> can be any value between the range depending on the hardware implementation.
>
>>> So I think we should patch this up in the vectorizer itself like with
>>> your patch. I'm going to wait for Richards input though since he
>>> seems to disagree.
>
> According tohttps://gcc.gnu.org/bugzilla/show_bug.cgi?id=109971, <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109971,>
> Kewen is happy with this patch, turns out this patch can fix power's issue.
Yeah, the exposed degradation and failures can be fixed by this patch.
I'd expect both approaches (this patch or extending niter analysis and
others) should work for the exposed issues.
A new finding is that my SPEC2017 rerun with this patch exposed some
verification failures, I made a regression test on Power10, it showed
a few failures too (mainly from fortran). By looking into one of them
(case gfortran.dg/array_alloc_2.f90), I think the patch needs some
adjustment on chosen code according to exit_edge->flags like:
diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
index ef28711c58f..5d518460b6d 100644
--- a/gcc/tree-vect-loop-manip.cc
+++ b/gcc/tree-vect-loop-manip.cc
@@ -892,8 +892,9 @@ vect_set_loop_condition_partial_vectors (class loop *loop,
if (LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo))
{
gcc_assert (compare_step);
- cond_stmt = gimple_build_cond (GT_EXPR, test_ctrl, compare_step,
- NULL_TREE, NULL_TREE);
+ tree_code code = (exit_edge->flags & EDGE_TRUE_VALUE) ? LE_EXPR : GT_EXPR;
+ cond_stmt = gimple_build_cond (code, test_ctrl, compare_step, NULL_TREE,
+ NULL_TREE);
gsi_insert_before (&loop_cond_gsi, cond_stmt, GSI_SAME_STMT);
}
else
I'm running regression testing again based on this adjustment, will see
if it can fix all exposed failures.
BR,
Kewen
> So, Let's wait for Richard's comments.
>
> Thanks.
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> juzhe.zhong@rivai.ai
>
>
> *From:* Richard Biener <mailto:rguenther@suse.de>
> *Date:* 2023-05-31 14:41
> *To:* juzhe.zhong@rivai.ai <mailto:juzhe.zhong@rivai.ai>
> *CC:* richard.sandiford <mailto:richard.sandiford@arm.com>; gcc-patches <mailto:gcc-patches@gcc.gnu.org>; linkw <mailto:linkw@linux.ibm.com>
> *Subject:* Re: Re: [PATCH] VECT: Change flow of decrement IV
> On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>
> > Hi?all. I have posted my several investigations:
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
> >
> > Turns out when "niters is a constant value and vf is a constant value"
> > This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> > Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
> >
> > Actually, for current flow:
> >
> > step = MIN ()
> > ...
> > remain = remain - step.
> >
> > I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> > So, could you make a decision for this patch?
> >
> > I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> > we should extend SCEV/IVOPTS ?
>
> I don't think we can do anything in SCEV for this which means we'd
> need to special-case this in niter analysis, in IVOPTs and any other
> passes that might be affected (and not fixed by handling it in niter
> analysis). While improving niter analysis would be good (the user
> could write this pattern as well) I do not have time to try
> implementing that (I have no idea how ugly or robust it is going to be).
>
> So I think we should patch this up in the vectorizer itself like with
> your patch. I'm going to wait for Richards input though since he
> seems to disagree.
>
> Note with SELECT_VL all bets will be off since as I understand the
> value it gives can vary from iteration to iteration (but we know
> a lower and maybe an upper bound?)
>
> Thanks,
> Richard.
>
> > Thanks.
> >
> >
> > juzhe.zhong@rivai.ai
> >
> > From: ???
> > Date: 2023-05-30 23:05
> > To: rguenther
> > CC: richard.sandiford; gcc-patches; linkw
> > Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
> > More information of power's testcase:
> >
> > Before this patch:
> > test_npeel_int16_t:
> > lui a4,%hi(.LANCHOR0+130)
> > lui a3,%hi(.LANCHOR1)
> > addi a3,a3,%lo(.LANCHOR1)
> > addi a4,a4,%lo(.LANCHOR0+130)
> > li a5,58
> > li a2,16
> > vsetivli zero,16,e16,m1,ta,ma
> > vl1re16.v v3,0(a3)
> > vid.v v1
> > .L5:
> > minu a3,a5,a2
> > vsetvli zero,a3,e16,m1,ta,ma
> > sub a5,a5,a3
> > vse16.v v1,0(a4)
> > vsetivli zero,16,e16,m1,ta,ma
> > addi a4,a4,32
> > vadd.vv v1,v1,v3
> > bne a5,zero,.L5
> > ret
> >
> > After this patch:
> > test_npeel_int16_t:
> > lui a5,%hi(.LANCHOR0)
> > addi a5,a5,%lo(.LANCHOR0)
> > li a1,16
> > vsetivli zero,16,e16,m1,ta,ma
> > addi a2,a5,130
> > vid.v v1
> > addi a3,a5,162
> > vadd.vx v4,v1,a1
> > addi a4,a5,194
> > li a1,32
> > vadd.vx v3,v1,a1
> > vse16.v v1,0(a2)
> > vse16.v v4,0(a3)
> > vse16.v v3,0(a4)
> > addi a5,a5,226
> > li a1,48
> > vadd.vx v2,v1,a1
> > vsetivli zero,10,e16,m1,ta,ma
> > vse16.v v2,0(a5)
> > ret
> >
> > It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
> >
> >
> > juzhe.zhong@rivai.ai
> >
> > From: Richard Biener
> > Date: 2023-05-30 20:33
> > To: juzhe.zhong
> > CC: Richard Sandiford; gcc-patches; linkw
> > Subject: Re: [PATCH] VECT: Change flow of decrement IV
> > On Tue, 30 May 2023, juzhe.zhong wrote:
> >
> > > This patch will generate the number of rgroup ?mov? instructions inside the
> > > loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> > > instruction in loop. If this patch is necessary? I think I should find a way
> > > to fix it.
> >
> > That's odd, you only need to adjust the IV which is used in the exit test,
> > not all the others.
> >
> > > ---- Replied Message ----
> > > From
> > > Richard Sandiford<richard.sandiford@arm.com>
> > > Date
> > > 05/30/2023 19:41
> > > To
> > > juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> > > Cc
> > > gcc-patches<gcc-patches@gcc.gnu.org>,
> > > rguenther<rguenther@suse.de>,
> > > linkw<linkw@linux.ibm.com>
> > > Subject
> > > Re: [PATCH] VECT: Change flow of decrement IV
> > > "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > > > Before this patch:
> > > > foo:
> > > > ble a2,zero,.L5
> > > > csrr a3,vlenb
> > > > srli a4,a3,2
> > > > .L3:
> > > > minu a5,a2,a4
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vle32.v v2,0(a1)
> > > > vle32.v v1,0(a0)
> > > > vsetvli t1,zero,e32,m1,ta,ma
> > > > vadd.vv v1,v1,v2
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vse32.v v1,0(a0)
> > > > add a1,a1,a3
> > > > add a0,a0,a3
> > > > sub a2,a2,a5
> > > > bne a2,zero,.L3
> > > > .L5:
> > > > ret
> > > >
> > > > After this patch:
> > > >
> > > > foo:
> > > > ble a2,zero,.L5
> > > > csrr a3,vlenb
> > > > srli a4,a3,2
> > > > neg a7,a4 -->>>additional instruction
> > > > .L3:
> > > > minu a5,a2,a4
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vle32.v v2,0(a1)
> > > > vle32.v v1,0(a0)
> > > > vsetvli t1,zero,e32,m1,ta,ma
> > > > mv a6,a2 -->>>additional instruction
> > > > vadd.vv v1,v1,v2
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vse32.v v1,0(a0)
> > > > add a1,a1,a3
> > > > add a0,a0,a3
> > > > add a2,a2,a7
> > > > bgtu a6,a4,.L3
> > > > .L5:
> > > > ret
> > > >
> > > > There is 1 more instruction in preheader and 1 more instruction in loop.
> > > > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> > > this issue will gone.
> > >
> > > But what about cases where you won't be using SELECT_VL, such as SLP?
> > >
> > > Richard
> > >
> > >
> >
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:38 ` Kewen.Lin
@ 2023-05-31 7:50 ` juzhe.zhong
0 siblings, 0 replies; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 7:50 UTC (permalink / raw)
To: linkw; +Cc: richard.sandiford, gcc-patches, rguenther
[-- Attachment #1: Type: text/plain, Size: 10663 bytes --]
Oh, it's correct fix. Thanks for catching this.
juzhe.zhong@rivai.ai
From: Kewen.Lin
Date: 2023-05-31 15:38
To: juzhe.zhong@rivai.ai
CC: richard.sandiford; gcc-patches; rguenther
Subject: Re: [PATCH] VECT: Change flow of decrement IV
> Hi, Richi.
>
>>> Note with SELECT_VL all bets will be off since as I understand the
>>> value it gives can vary from iteration to iteration (but we know
>>> a lower and maybe an upper bound?)
> Yes, in RVV side, the SELECT_VL output can be in range of [ceil(avl/2), vlmax],
> can be any value between the range depending on the hardware implementation.
>
>>> So I think we should patch this up in the vectorizer itself like with
>>> your patch. I'm going to wait for Richards input though since he
>>> seems to disagree.
>
> According tohttps://gcc.gnu.org/bugzilla/show_bug.cgi?id=109971, <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109971,>
> Kewen is happy with this patch, turns out this patch can fix power's issue.
Yeah, the exposed degradation and failures can be fixed by this patch.
I'd expect both approaches (this patch or extending niter analysis and
others) should work for the exposed issues.
A new finding is that my SPEC2017 rerun with this patch exposed some
verification failures, I made a regression test on Power10, it showed
a few failures too (mainly from fortran). By looking into one of them
(case gfortran.dg/array_alloc_2.f90), I think the patch needs some
adjustment on chosen code according to exit_edge->flags like:
diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
index ef28711c58f..5d518460b6d 100644
--- a/gcc/tree-vect-loop-manip.cc
+++ b/gcc/tree-vect-loop-manip.cc
@@ -892,8 +892,9 @@ vect_set_loop_condition_partial_vectors (class loop *loop,
if (LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo))
{
gcc_assert (compare_step);
- cond_stmt = gimple_build_cond (GT_EXPR, test_ctrl, compare_step,
- NULL_TREE, NULL_TREE);
+ tree_code code = (exit_edge->flags & EDGE_TRUE_VALUE) ? LE_EXPR : GT_EXPR;
+ cond_stmt = gimple_build_cond (code, test_ctrl, compare_step, NULL_TREE,
+ NULL_TREE);
gsi_insert_before (&loop_cond_gsi, cond_stmt, GSI_SAME_STMT);
}
else
I'm running regression testing again based on this adjustment, will see
if it can fix all exposed failures.
BR,
Kewen
> So, Let's wait for Richard's comments.
>
> Thanks.
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> juzhe.zhong@rivai.ai
>
>
> *From:* Richard Biener <mailto:rguenther@suse.de>
> *Date:* 2023-05-31 14:41
> *To:* juzhe.zhong@rivai.ai <mailto:juzhe.zhong@rivai.ai>
> *CC:* richard.sandiford <mailto:richard.sandiford@arm.com>; gcc-patches <mailto:gcc-patches@gcc.gnu.org>; linkw <mailto:linkw@linux.ibm.com>
> *Subject:* Re: Re: [PATCH] VECT: Change flow of decrement IV
> On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>
> > Hi?all. I have posted my several investigations:
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
> >
> > Turns out when "niters is a constant value and vf is a constant value"
> > This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> > Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
> >
> > Actually, for current flow:
> >
> > step = MIN ()
> > ...
> > remain = remain - step.
> >
> > I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> > So, could you make a decision for this patch?
> >
> > I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> > we should extend SCEV/IVOPTS ?
>
> I don't think we can do anything in SCEV for this which means we'd
> need to special-case this in niter analysis, in IVOPTs and any other
> passes that might be affected (and not fixed by handling it in niter
> analysis). While improving niter analysis would be good (the user
> could write this pattern as well) I do not have time to try
> implementing that (I have no idea how ugly or robust it is going to be).
>
> So I think we should patch this up in the vectorizer itself like with
> your patch. I'm going to wait for Richards input though since he
> seems to disagree.
>
> Note with SELECT_VL all bets will be off since as I understand the
> value it gives can vary from iteration to iteration (but we know
> a lower and maybe an upper bound?)
>
> Thanks,
> Richard.
>
> > Thanks.
> >
> >
> > juzhe.zhong@rivai.ai
> >
> > From: ???
> > Date: 2023-05-30 23:05
> > To: rguenther
> > CC: richard.sandiford; gcc-patches; linkw
> > Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
> > More information of power's testcase:
> >
> > Before this patch:
> > test_npeel_int16_t:
> > lui a4,%hi(.LANCHOR0+130)
> > lui a3,%hi(.LANCHOR1)
> > addi a3,a3,%lo(.LANCHOR1)
> > addi a4,a4,%lo(.LANCHOR0+130)
> > li a5,58
> > li a2,16
> > vsetivli zero,16,e16,m1,ta,ma
> > vl1re16.v v3,0(a3)
> > vid.v v1
> > .L5:
> > minu a3,a5,a2
> > vsetvli zero,a3,e16,m1,ta,ma
> > sub a5,a5,a3
> > vse16.v v1,0(a4)
> > vsetivli zero,16,e16,m1,ta,ma
> > addi a4,a4,32
> > vadd.vv v1,v1,v3
> > bne a5,zero,.L5
> > ret
> >
> > After this patch:
> > test_npeel_int16_t:
> > lui a5,%hi(.LANCHOR0)
> > addi a5,a5,%lo(.LANCHOR0)
> > li a1,16
> > vsetivli zero,16,e16,m1,ta,ma
> > addi a2,a5,130
> > vid.v v1
> > addi a3,a5,162
> > vadd.vx v4,v1,a1
> > addi a4,a5,194
> > li a1,32
> > vadd.vx v3,v1,a1
> > vse16.v v1,0(a2)
> > vse16.v v4,0(a3)
> > vse16.v v3,0(a4)
> > addi a5,a5,226
> > li a1,48
> > vadd.vx v2,v1,a1
> > vsetivli zero,10,e16,m1,ta,ma
> > vse16.v v2,0(a5)
> > ret
> >
> > It's obvious, previously, power's testcase in RVV side can not unroll, but after this patch, in RVV side, it can unroll now.
> >
> >
> > juzhe.zhong@rivai.ai
> >
> > From: Richard Biener
> > Date: 2023-05-30 20:33
> > To: juzhe.zhong
> > CC: Richard Sandiford; gcc-patches; linkw
> > Subject: Re: [PATCH] VECT: Change flow of decrement IV
> > On Tue, 30 May 2023, juzhe.zhong wrote:
> >
> > > This patch will generate the number of rgroup ?mov? instructions inside the
> > > loop. This is unacceptable. For example?if number of rgroups=3? will be 3 more
> > > instruction in loop. If this patch is necessary? I think I should find a way
> > > to fix it.
> >
> > That's odd, you only need to adjust the IV which is used in the exit test,
> > not all the others.
> >
> > > ---- Replied Message ----
> > > From
> > > Richard Sandiford<richard.sandiford@arm.com>
> > > Date
> > > 05/30/2023 19:41
> > > To
> > > juzhe.zhong@rivai.ai<juzhe.zhong@rivai.ai>
> > > Cc
> > > gcc-patches<gcc-patches@gcc.gnu.org>,
> > > rguenther<rguenther@suse.de>,
> > > linkw<linkw@linux.ibm.com>
> > > Subject
> > > Re: [PATCH] VECT: Change flow of decrement IV
> > > "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> writes:
> > > > Before this patch:
> > > > foo:
> > > > ble a2,zero,.L5
> > > > csrr a3,vlenb
> > > > srli a4,a3,2
> > > > .L3:
> > > > minu a5,a2,a4
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vle32.v v2,0(a1)
> > > > vle32.v v1,0(a0)
> > > > vsetvli t1,zero,e32,m1,ta,ma
> > > > vadd.vv v1,v1,v2
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vse32.v v1,0(a0)
> > > > add a1,a1,a3
> > > > add a0,a0,a3
> > > > sub a2,a2,a5
> > > > bne a2,zero,.L3
> > > > .L5:
> > > > ret
> > > >
> > > > After this patch:
> > > >
> > > > foo:
> > > > ble a2,zero,.L5
> > > > csrr a3,vlenb
> > > > srli a4,a3,2
> > > > neg a7,a4 -->>>additional instruction
> > > > .L3:
> > > > minu a5,a2,a4
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vle32.v v2,0(a1)
> > > > vle32.v v1,0(a0)
> > > > vsetvli t1,zero,e32,m1,ta,ma
> > > > mv a6,a2 -->>>additional instruction
> > > > vadd.vv v1,v1,v2
> > > > vsetvli zero,a5,e32,m1,ta,ma
> > > > vse32.v v1,0(a0)
> > > > add a1,a1,a3
> > > > add a0,a0,a3
> > > > add a2,a2,a7
> > > > bgtu a6,a4,.L3
> > > > .L5:
> > > > ret
> > > >
> > > > There is 1 more instruction in preheader and 1 more instruction in loop.
> > > > But I think it's OK for RVV since we will definitely be using SELECT_VL so
> > > this issue will gone.
> > >
> > > But what about cases where you won't be using SELECT_VL, such as SLP?
> > >
> > > Richard
> > >
> > >
> >
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 6:41 ` Richard Biener
2023-05-31 6:50 ` juzhe.zhong
@ 2023-05-31 7:28 ` Richard Sandiford
2023-05-31 7:36 ` juzhe.zhong
2023-05-31 7:38 ` Richard Biener
1 sibling, 2 replies; 26+ messages in thread
From: Richard Sandiford @ 2023-05-31 7:28 UTC (permalink / raw)
To: Richard Biener; +Cc: juzhe.zhong, gcc-patches, linkw
Richard Biener <rguenther@suse.de> writes:
> On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>
>> Hi?all. I have posted my several investigations:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>>
>> Turns out when "niters is a constant value and vf is a constant value"
>> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
>> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>>
>> Actually, for current flow:
>>
>> step = MIN ()
>> ...
>> remain = remain - step.
>>
>> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
>> So, could you make a decision for this patch?
>>
>> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
>> we should extend SCEV/IVOPTS ?
>
> I don't think we can do anything in SCEV for this which means we'd
> need to special-case this in niter analysis, in IVOPTs and any other
> passes that might be affected (and not fixed by handling it in niter
> analysis). While improving niter analysis would be good (the user
> could write this pattern as well) I do not have time to try
> implementing that (I have no idea how ugly or robust it is going to be).
>
> So I think we should patch this up in the vectorizer itself like with
> your patch. I'm going to wait for Richards input though since he
> seems to disagree.
I think my main disagreement is that the IV phi can be analysed
as a SCEV with sufficient work (realising that the MIN result is
always VF when the latch is executed). That SCEV might be useful
“as is” for things like IVOPTS, without specific work in those passes.
(Although perhaps not too useful, since most other IVs will be upcounting.)
I don't object though. It just feels like we're giving up easily.
And that's a bit frustrating, since this potential problem was flagged
ahead of time.
> Note with SELECT_VL all bets will be off since as I understand the
> value it gives can vary from iteration to iteration (but we know
> a lower and maybe an upper bound?)
Right. All IVs will have a variable step for SELECT_VL.
Thanks,
Richard
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:28 ` Richard Sandiford
@ 2023-05-31 7:36 ` juzhe.zhong
2023-05-31 8:44 ` Richard Biener
2023-05-31 7:38 ` Richard Biener
1 sibling, 1 reply; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 7:36 UTC (permalink / raw)
To: richard.sandiford, rguenther; +Cc: gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]
Hi, Richard.
>> I don't object though. It just feels like we're giving up easily.
>> And that's a bit frustrating, since this potential problem was flagged
>> ahead of time.
I can take a look at it. Would you mind giving me some hints?
Should I do this in which PASS ? "ivopts" PASS?
Is that right that we can enhance analysis when we see the statement as follows:
remain = remain - step and step is coming from a MIN_EXPR (remain, vf).
Then what we need to do?
Thanks.
juzhe.zhong@rivai.ai
From: Richard Sandiford
Date: 2023-05-31 15:28
To: Richard Biener
CC: juzhe.zhong\@rivai.ai; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
Richard Biener <rguenther@suse.de> writes:
> On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>
>> Hi?all. I have posted my several investigations:
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>>
>> Turns out when "niters is a constant value and vf is a constant value"
>> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
>> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>>
>> Actually, for current flow:
>>
>> step = MIN ()
>> ...
>> remain = remain - step.
>>
>> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
>> So, could you make a decision for this patch?
>>
>> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
>> we should extend SCEV/IVOPTS ?
>
> I don't think we can do anything in SCEV for this which means we'd
> need to special-case this in niter analysis, in IVOPTs and any other
> passes that might be affected (and not fixed by handling it in niter
> analysis). While improving niter analysis would be good (the user
> could write this pattern as well) I do not have time to try
> implementing that (I have no idea how ugly or robust it is going to be).
>
> So I think we should patch this up in the vectorizer itself like with
> your patch. I'm going to wait for Richards input though since he
> seems to disagree.
I think my main disagreement is that the IV phi can be analysed
as a SCEV with sufficient work (realising that the MIN result is
always VF when the latch is executed). That SCEV might be useful
“as is” for things like IVOPTS, without specific work in those passes.
(Although perhaps not too useful, since most other IVs will be upcounting.)
I don't object though. It just feels like we're giving up easily.
And that's a bit frustrating, since this potential problem was flagged
ahead of time.
> Note with SELECT_VL all bets will be off since as I understand the
> value it gives can vary from iteration to iteration (but we know
> a lower and maybe an upper bound?)
Right. All IVs will have a variable step for SELECT_VL.
Thanks,
Richard
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:36 ` juzhe.zhong
@ 2023-05-31 8:44 ` Richard Biener
0 siblings, 0 replies; 26+ messages in thread
From: Richard Biener @ 2023-05-31 8:44 UTC (permalink / raw)
To: juzhe.zhong; +Cc: richard.sandiford, gcc-patches, linkw
On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> Hi, Richard.
>
> >> I don't object though. It just feels like we're giving up easily.
> >> And that's a bit frustrating, since this potential problem was flagged
> >> ahead of time.
>
> I can take a look at it. Would you mind giving me some hints?
> Should I do this in which PASS ? "ivopts" PASS?
> Is that right that we can enhance analysis when we see the statement as follows:
> remain = remain - step and step is coming from a MIN_EXPR (remain, vf).
> Then what we need to do?
The key is that we have
# iv = PHI <initial, iv'>
step = MIN (iv, invariant);
iv' = iv - step;
if (iv' != 0)
continue;
in that case for the purpose of niter analysis we can ignore
the MIN expression and use 'invariant' as step (whether constant
or not). Of course it only works for unsigned 'step'.
niter analysis uses simple_iv () but that necessarily fails here
so one idea would be to enhance simple_iv when passed a special
flag. Another idea is to add to the set of patterns niter analysis
already has to compute niters resolving to popcount and friends
and pattern match the above (I'd start with that to see which other
passes / analyses besides niter analysis need improvement).
number_of_iterations_exit_assumptions does
if (!simple_iv_with_niters (loop, loop_containing_stmt (stmt),
op0, &iv0, safe ? &iv0_niters : NULL,
false))
return number_of_iterations_bitcount (loop, exit, code, niter);
so I'd add that to the set of matchers in number_of_iterations_bitcount
(and maybe rename that to number_of_iterations_pattern).
Richard.
> Thanks.
>
>
> juzhe.zhong@rivai.ai
>
> From: Richard Sandiford
> Date: 2023-05-31 15:28
> To: Richard Biener
> CC: juzhe.zhong\@rivai.ai; gcc-patches; linkw
> Subject: Re: [PATCH] VECT: Change flow of decrement IV
> Richard Biener <rguenther@suse.de> writes:
> > On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> >
> >> Hi?all. I have posted my several investigations:
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
> >>
> >> Turns out when "niters is a constant value and vf is a constant value"
> >> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> >> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
> >>
> >> Actually, for current flow:
> >>
> >> step = MIN ()
> >> ...
> >> remain = remain - step.
> >>
> >> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> >> So, could you make a decision for this patch?
> >>
> >> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> >> we should extend SCEV/IVOPTS ?
> >
> > I don't think we can do anything in SCEV for this which means we'd
> > need to special-case this in niter analysis, in IVOPTs and any other
> > passes that might be affected (and not fixed by handling it in niter
> > analysis). While improving niter analysis would be good (the user
> > could write this pattern as well) I do not have time to try
> > implementing that (I have no idea how ugly or robust it is going to be).
> >
> > So I think we should patch this up in the vectorizer itself like with
> > your patch. I'm going to wait for Richards input though since he
> > seems to disagree.
>
> I think my main disagreement is that the IV phi can be analysed
> as a SCEV with sufficient work (realising that the MIN result is
> always VF when the latch is executed). That SCEV might be useful
> ?as is? for things like IVOPTS, without specific work in those passes.
> (Although perhaps not too useful, since most other IVs will be upcounting.)
>
> I don't object though. It just feels like we're giving up easily.
> And that's a bit frustrating, since this potential problem was flagged
> ahead of time.
>
> > Note with SELECT_VL all bets will be off since as I understand the
> > value it gives can vary from iteration to iteration (but we know
> > a lower and maybe an upper bound?)
>
> Right. All IVs will have a variable step for SELECT_VL.
>
> Thanks,
> Richard
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:28 ` Richard Sandiford
2023-05-31 7:36 ` juzhe.zhong
@ 2023-05-31 7:38 ` Richard Biener
2023-05-31 7:49 ` juzhe.zhong
2023-05-31 9:01 ` Richard Sandiford
1 sibling, 2 replies; 26+ messages in thread
From: Richard Biener @ 2023-05-31 7:38 UTC (permalink / raw)
To: Richard Sandiford; +Cc: juzhe.zhong, gcc-patches, linkw
On Wed, 31 May 2023, Richard Sandiford wrote:
> Richard Biener <rguenther@suse.de> writes:
> > On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> >
> >> Hi?all. I have posted my several investigations:
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
> >>
> >> Turns out when "niters is a constant value and vf is a constant value"
> >> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> >> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
> >>
> >> Actually, for current flow:
> >>
> >> step = MIN ()
> >> ...
> >> remain = remain - step.
> >>
> >> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> >> So, could you make a decision for this patch?
> >>
> >> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> >> we should extend SCEV/IVOPTS ?
> >
> > I don't think we can do anything in SCEV for this which means we'd
> > need to special-case this in niter analysis, in IVOPTs and any other
> > passes that might be affected (and not fixed by handling it in niter
> > analysis). While improving niter analysis would be good (the user
> > could write this pattern as well) I do not have time to try
> > implementing that (I have no idea how ugly or robust it is going to be).
> >
> > So I think we should patch this up in the vectorizer itself like with
> > your patch. I'm going to wait for Richards input though since he
> > seems to disagree.
>
> I think my main disagreement is that the IV phi can be analysed
> as a SCEV with sufficient work (realising that the MIN result is
> always VF when the latch is executed). That SCEV might be useful
> ?as is? for things like IVOPTS, without specific work in those passes.
> (Although perhaps not too useful, since most other IVs will be upcounting.)
I think we'd need another API for SCEV there then,
analyze_scalar_evolution_for_latch () so we can disregard the
value on the exit edges then. That means we'd still need to touch
all users and decide whether it's safe to use that or not.
> I don't object though. It just feels like we're giving up easily.
> And that's a bit frustrating, since this potential problem was flagged
> ahead of time.
Well, I expect that massaging SCEV and niter analysis will take
up quite some developer time while avoiding the situation in
the vectorizer is possible (and would fix the observed regressions).
We can always improve later here and I'd suggest to file an
enhancement bugreport with a simple C testcase using this kind of
iteration.
I'm just saying that to go forward the vectorizer change looks
more promising (also considering the pace RISC-V people are working at
...)
Richard.
> > Note with SELECT_VL all bets will be off since as I understand the
> > value it gives can vary from iteration to iteration (but we know
> > a lower and maybe an upper bound?)
>
> Right. All IVs will have a variable step for SELECT_VL.
>
> Thanks,
> Richard
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:38 ` Richard Biener
@ 2023-05-31 7:49 ` juzhe.zhong
2023-05-31 9:01 ` Richard Sandiford
1 sibling, 0 replies; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 7:49 UTC (permalink / raw)
To: rguenther, richard.sandiford; +Cc: gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 4269 bytes --]
>> I'm just saying that to go forward the vectorizer change looks
>>more promising (also considering the pace RISC-V people are working at
>>...)
Yeah, RVV needs a lot of middle-end support:
SELECT_VL, LEN_MASK_LOAD/LEN_MASK_STORE,.....etc
LEN_ADD for RVV reduction support like COND_ADD for ARM SVE...etc
SELECT_VL is still pending.
Without support in middle-end, GCC can not support powerful auto-vectorization (Performance will be much worse than RVV LLVM).
And unfortunately, I am the only guy working on middle-end support of RVV auto-vectorization. :)
I think we can make this patch merged and record the enhancement of SCEV in bugzilla to see we can improve that in the future.
Thanks.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-31 15:38
To: Richard Sandiford
CC: juzhe.zhong@rivai.ai; gcc-patches; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
On Wed, 31 May 2023, Richard Sandiford wrote:
> Richard Biener <rguenther@suse.de> writes:
> > On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> >
> >> Hi?all. I have posted my several investigations:
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
> >>
> >> Turns out when "niters is a constant value and vf is a constant value"
> >> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
> >> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
> >>
> >> Actually, for current flow:
> >>
> >> step = MIN ()
> >> ...
> >> remain = remain - step.
> >>
> >> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
> >> So, could you make a decision for this patch?
> >>
> >> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
> >> we should extend SCEV/IVOPTS ?
> >
> > I don't think we can do anything in SCEV for this which means we'd
> > need to special-case this in niter analysis, in IVOPTs and any other
> > passes that might be affected (and not fixed by handling it in niter
> > analysis). While improving niter analysis would be good (the user
> > could write this pattern as well) I do not have time to try
> > implementing that (I have no idea how ugly or robust it is going to be).
> >
> > So I think we should patch this up in the vectorizer itself like with
> > your patch. I'm going to wait for Richards input though since he
> > seems to disagree.
>
> I think my main disagreement is that the IV phi can be analysed
> as a SCEV with sufficient work (realising that the MIN result is
> always VF when the latch is executed). That SCEV might be useful
> ?as is? for things like IVOPTS, without specific work in those passes.
> (Although perhaps not too useful, since most other IVs will be upcounting.)
I think we'd need another API for SCEV there then,
analyze_scalar_evolution_for_latch () so we can disregard the
value on the exit edges then. That means we'd still need to touch
all users and decide whether it's safe to use that or not.
> I don't object though. It just feels like we're giving up easily.
> And that's a bit frustrating, since this potential problem was flagged
> ahead of time.
Well, I expect that massaging SCEV and niter analysis will take
up quite some developer time while avoiding the situation in
the vectorizer is possible (and would fix the observed regressions).
We can always improve later here and I'd suggest to file an
enhancement bugreport with a simple C testcase using this kind of
iteration.
I'm just saying that to go forward the vectorizer change looks
more promising (also considering the pace RISC-V people are working at
...)
Richard.
> > Note with SELECT_VL all bets will be off since as I understand the
> > value it gives can vary from iteration to iteration (but we know
> > a lower and maybe an upper bound?)
>
> Right. All IVs will have a variable step for SELECT_VL.
>
> Thanks,
> Richard
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 7:38 ` Richard Biener
2023-05-31 7:49 ` juzhe.zhong
@ 2023-05-31 9:01 ` Richard Sandiford
2023-05-31 9:30 ` juzhe.zhong
1 sibling, 1 reply; 26+ messages in thread
From: Richard Sandiford @ 2023-05-31 9:01 UTC (permalink / raw)
To: Richard Biener via Gcc-patches; +Cc: Richard Biener, juzhe.zhong, linkw
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Wed, 31 May 2023, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>> >
>> >> Hi?all. I have posted my several investigations:
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>> >>
>> >> Turns out when "niters is a constant value and vf is a constant value"
>> >> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
>> >> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>> >>
>> >> Actually, for current flow:
>> >>
>> >> step = MIN ()
>> >> ...
>> >> remain = remain - step.
>> >>
>> >> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
>> >> So, could you make a decision for this patch?
>> >>
>> >> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
>> >> we should extend SCEV/IVOPTS ?
>> >
>> > I don't think we can do anything in SCEV for this which means we'd
>> > need to special-case this in niter analysis, in IVOPTs and any other
>> > passes that might be affected (and not fixed by handling it in niter
>> > analysis). While improving niter analysis would be good (the user
>> > could write this pattern as well) I do not have time to try
>> > implementing that (I have no idea how ugly or robust it is going to be).
>> >
>> > So I think we should patch this up in the vectorizer itself like with
>> > your patch. I'm going to wait for Richards input though since he
>> > seems to disagree.
>>
>> I think my main disagreement is that the IV phi can be analysed
>> as a SCEV with sufficient work (realising that the MIN result is
>> always VF when the latch is executed). That SCEV might be useful
>> ?as is? for things like IVOPTS, without specific work in those passes.
>> (Although perhaps not too useful, since most other IVs will be upcounting.)
>
> I think we'd need another API for SCEV there then,
> analyze_scalar_evolution_for_latch () so we can disregard the
> value on the exit edges then. That means we'd still need to touch
> all users and decide whether it's safe to use that or not.
I'd expect the phi for the IV with the constant step to have the same
value as the phi for the IV with a MIN step. I realise that the phi
isn't the thing that matters for niters, but I'd expect IVOPTS to
consider both the phi and the adjusted value to be candidates. Only the
phi can be a candidate with the MIN step, but it feels like it should
still be a candidate, even with current interfaces.
You know this stuff much better than I do though, so I^m almost
certainly oversimplifying/overlooking things.
Like I say, I don't object to the vectoriser change, so please
don't go down a rabbit hole on my account. :)
Thanks,
Richard
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 9:01 ` Richard Sandiford
@ 2023-05-31 9:30 ` juzhe.zhong
2023-05-31 10:53 ` Richard Biener
0 siblings, 1 reply; 26+ messages in thread
From: juzhe.zhong @ 2023-05-31 9:30 UTC (permalink / raw)
To: richard.sandiford, gcc-patches; +Cc: rguenther, linkw
[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]
Thanks Richard.
Seems that this patch's approach is ok to trunk?
Maybe the only thing we should do is to wait Kewen's testing feedback, am I right ?
Thanks.
juzhe.zhong@rivai.ai
From: Richard Sandiford
Date: 2023-05-31 17:01
To: Richard Biener via Gcc-patches
CC: Richard Biener; juzhe.zhong\@rivai.ai; linkw
Subject: Re: [PATCH] VECT: Change flow of decrement IV
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Wed, 31 May 2023, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
>> >
>> >> Hi?all. I have posted my several investigations:
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620101.html
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620105.html
>> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620108.html
>> >>
>> >> Turns out when "niters is a constant value and vf is a constant value"
>> >> This patch can allow SCEV/IVOPTS optimize a lot for RVV too (Take tesecase from IBM's testsuite for example) and I think this patch can fix IBM's cunroll issue.
>> >> Even though it will produce a 'mv' instruction in some ohter cases for RVV, I think Gain > Pain overal.
>> >>
>> >> Actually, for current flow:
>> >>
>> >> step = MIN ()
>> >> ...
>> >> remain = remain - step.
>> >>
>> >> I don't know how difficult to extend SCEV/IVOPTS to fix this issue.
>> >> So, could you make a decision for this patch?
>> >>
>> >> I wonder whether we should apply the approach of this patch (the codes can be refined after well reviewed) or
>> >> we should extend SCEV/IVOPTS ?
>> >
>> > I don't think we can do anything in SCEV for this which means we'd
>> > need to special-case this in niter analysis, in IVOPTs and any other
>> > passes that might be affected (and not fixed by handling it in niter
>> > analysis). While improving niter analysis would be good (the user
>> > could write this pattern as well) I do not have time to try
>> > implementing that (I have no idea how ugly or robust it is going to be).
>> >
>> > So I think we should patch this up in the vectorizer itself like with
>> > your patch. I'm going to wait for Richards input though since he
>> > seems to disagree.
>>
>> I think my main disagreement is that the IV phi can be analysed
>> as a SCEV with sufficient work (realising that the MIN result is
>> always VF when the latch is executed). That SCEV might be useful
>> ?as is? for things like IVOPTS, without specific work in those passes.
>> (Although perhaps not too useful, since most other IVs will be upcounting.)
>
> I think we'd need another API for SCEV there then,
> analyze_scalar_evolution_for_latch () so we can disregard the
> value on the exit edges then. That means we'd still need to touch
> all users and decide whether it's safe to use that or not.
I'd expect the phi for the IV with the constant step to have the same
value as the phi for the IV with a MIN step. I realise that the phi
isn't the thing that matters for niters, but I'd expect IVOPTS to
consider both the phi and the adjusted value to be candidates. Only the
phi can be a candidate with the MIN step, but it feels like it should
still be a candidate, even with current interfaces.
You know this stuff much better than I do though, so I^m almost
certainly oversimplifying/overlooking things.
Like I say, I don't object to the vectoriser change, so please
don't go down a rabbit hole on my account. :)
Thanks,
Richard
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 9:30 ` juzhe.zhong
@ 2023-05-31 10:53 ` Richard Biener
2023-05-31 12:16 ` 钟居哲
0 siblings, 1 reply; 26+ messages in thread
From: Richard Biener @ 2023-05-31 10:53 UTC (permalink / raw)
To: juzhe.zhong; +Cc: richard.sandiford, gcc-patches, linkw
On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> Thanks Richard.
> Seems that this patch's approach is ok to trunk?
> Maybe the only thing we should do is to wait Kewen's testing feedback, am I right ?
Can you repost the patch with Kevens fix and state how you tested it?
Thanks,
Richard.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Re: [PATCH] VECT: Change flow of decrement IV
2023-05-31 10:53 ` Richard Biener
@ 2023-05-31 12:16 ` 钟居哲
0 siblings, 0 replies; 26+ messages in thread
From: 钟居哲 @ 2023-05-31 12:16 UTC (permalink / raw)
To: rguenther; +Cc: richard.sandiford, gcc-patches, linkw
[-- Attachment #1: Type: text/plain, Size: 897 bytes --]
Sure. I will repost it with kewen's fix.
Previously, this patch I tested on X86 and apply to upstream RVV gcc tested on upstream RVV testsuite.
Since the upstream RVV GCC is not ready and fragile, we can't use it compile big program.
I will apply this patch on my downstream GCC and do my downstream regression (Regression is very big and include so many benchmarks).
Thanks.
juzhe.zhong@rivai.ai
From: Richard Biener
Date: 2023-05-31 18:53
To: juzhe.zhong@rivai.ai
CC: richard.sandiford; gcc-patches; linkw
Subject: Re: Re: [PATCH] VECT: Change flow of decrement IV
On Wed, 31 May 2023, juzhe.zhong@rivai.ai wrote:
> Thanks Richard.
> Seems that this patch's approach is ok to trunk?
> Maybe the only thing we should do is to wait Kewen's testing feedback, am I right ?
Can you repost the patch with Kevens fix and state how you tested it?
Thanks,
Richard.
^ permalink raw reply [flat|nested] 26+ messages in thread