[committed] RISC-V: Add crypto vector builtin function.

[committed] RISC-V: Add crypto vector builtin function.

[juzhe.zhong]

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

Hi, Wang Feng.

Your patch has some ICEs:
FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for excess errors)
FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for excess errors)

I suspect you didn't enable rtl check in the regression:

../../configure --enable-gcc-checking=rtl.
Plz enable rtl check in the regression tests.



juzhe.zhong@rivai.ai

Re: [committed] RISC-V: Add crypto vector builtin function.

[Palmer Dabbelt]

On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
> Hi, Wang Feng.
> 
> Your patch has some ICEs:
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for excess errors)

So let's just revert it, it doesn't even look like it was reviewed.  
We've set a really bad precedent here where we're just merging a bunch 
of unreviewed code and sorting out the regressions in trunk, that's not 
the right way to do things.

> 
> I suspect you didn't enable rtl check in the regression:
> 
> ../../configure --enable-gcc-checking=rtl.
> Plz enable rtl check in the regression tests.
> 
> 
> 
> juzhe.zhong@rivai.ai

Re: Re: [committed] RISC-V: Add crypto vector builtin function.

[juzhe.zhong]

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

We (me and kito) has reviewed vector-crypto.

I believe Wang Feng has done && passed the regression (with no RTL check), but he just didn't enable RTL check I guessed.
(By default, RTL check is disabled in riscv-gnu-toolchain, developers need to enable it manually).

It's ok to revert the patch, then commit it after he fixes the ICE with enabling RTL check.



juzhe.zhong@rivai.ai
 
From: Palmer Dabbelt
Date: 2024-01-05 11:24
To: juzhe.zhong
CC: gcc-patches; Kito Cheng; Kito.cheng
Subject: Re: [committed] RISC-V: Add crypto vector builtin function.
On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
> Hi, Wang Feng.
> 
> Your patch has some ICEs:
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for excess errors)
 
So let's just revert it, it doesn't even look like it was reviewed.  
We've set a really bad precedent here where we're just merging a bunch 
of unreviewed code and sorting out the regressions in trunk, that's not 
the right way to do things.
 
> 
> I suspect you didn't enable rtl check in the regression:
> 
> ../../configure --enable-gcc-checking=rtl.
> Plz enable rtl check in the regression tests.
> 
> 
> 
> juzhe.zhong@rivai.ai
 

Re: Re: [committed] RISC-V: Add crypto vector builtin function.

[juzhe.zhong]

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

I have reverted those 2 commits:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=960c2620db254a1edc2cd61e608df73073b3de0d 
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b3ec98d458f2b285bb7b3fa4fcd93fd830fee069 





juzhe.zhong@rivai.ai
 
From: Palmer Dabbelt
Date: 2024-01-05 11:24
To: juzhe.zhong
CC: gcc-patches; Kito Cheng; Kito.cheng
Subject: Re: [committed] RISC-V: Add crypto vector builtin function.
On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
> Hi, Wang Feng.
> 
> Your patch has some ICEs:
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for excess errors)
 
So let's just revert it, it doesn't even look like it was reviewed.  
We've set a really bad precedent here where we're just merging a bunch 
of unreviewed code and sorting out the regressions in trunk, that's not 
the right way to do things.
 
> 
> I suspect you didn't enable rtl check in the regression:
> 
> ../../configure --enable-gcc-checking=rtl.
> Plz enable rtl check in the regression tests.
> 
> 
> 
> juzhe.zhong@rivai.ai
 

Re: Re: [committed] RISC-V: Add crypto vector builtin function.

[Feng Wang]

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

Yes,  as Juzhe said I had run all the riscv regression, but without RTL check. I will fix it.
Sorry for forgetting to enable RTL checking for testing.
Thanks Juzhe for helping me reverse this commit.
 
From: juzhe.zhong@rivai.ai
Date: 2024-01-05 11:28
To: palmer
CC: gcc-patches; kito.cheng; Kito.cheng
Subject: Re: Re: [committed] RISC-V: Add crypto vector builtin function.
We (me and kito) has reviewed vector-crypto.

I believe Wang Feng has done && passed the regression (with no RTL check), but he just didn't enable RTL check I guessed.
(By default, RTL check is disabled in riscv-gnu-toolchain, developers need to enable it manually).

It's ok to revert the patch, then commit it after he fixes the ICE with enabling RTL check.



juzhe.zhong@rivai.ai
 
From: Palmer Dabbelt
Date: 2024-01-05 11:24
To: juzhe.zhong
CC: gcc-patches; Kito Cheng; Kito.cheng
Subject: Re: [committed] RISC-V: Add crypto vector builtin function.
On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
> Hi, Wang Feng.
> 
> Your patch has some ICEs:
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for excess errors)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal compiler error: RTL check: expected code 'const_int', have 'reg' in vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for excess errors)
 
So let's just revert it, it doesn't even look like it was reviewed.  
We've set a really bad precedent here where we're just merging a bunch 
of unreviewed code and sorting out the regressions in trunk, that's not 
the right way to do things.
 
> 
> I suspect you didn't enable rtl check in the regression:
> 
> ../../configure --enable-gcc-checking=rtl.
> Plz enable rtl check in the regression tests.
> 
> 
> 
> juzhe.zhong@rivai.ai
 

Re: [committed] RISC-V: Add crypto vector builtin function.

[Jeff Law]

On 1/4/24 20:24, Palmer Dabbelt wrote:
> On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
>> Hi, Wang Feng.
>>
>> Your patch has some ICEs:
>> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler 
>> error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal 
>> compiler error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for 
>> excess errors)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal 
>> compiler error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for 
>> excess errors)
> 
> So let's just revert it, it doesn't even look like it was reviewed. 
> We've set a really bad precedent here where we're just merging a bunch 
> of unreviewed code and sorting out the regressions in trunk, that's not 
> the right way to do things.
> 
>>
>> I suspect you didn't enable rtl check in the regression:
>>
>> ../../configure --enable-gcc-checking=rtl.
>> Plz enable rtl check in the regression tests.
We haven't ever required folks to test with RTL checking enabled due to 
its compile-time cost.  So I don't think Feng did anything wrong here.

IIRC, Jakub's standard practice over in the x86 world is to do a 
bootstrap and regression test with RTL checking enabled in the spring as 
we get closer to the release to weed out these kinds of things that can 
slip through.

Clearly there's a bug and we should fix it, but it's not a sign that 
anything has gone terribly wrong.

jeff

Re: Re: [committed] RISC-V: Add crypto vector builtin function.

[钟居哲]

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

Thanks Jeff.

Yeah, I aggree we are not doing thing terribly wrong but Palmer request revert of the vector-crypto,
so I revert it (actually, I asked Li Pan revert it).

Actually, Wang Feng has fixed the issue:
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/641903.html 
It's just a pretty simple typo cause the ICE.

Soon, vector-crypto will be committed.

Eswin guys are working on various vector extension (vector crypto, BF16 vector, ...etc).
And I have told them only vector-crypto can accepted in the GCC-14 release and defer BF16 vector to GCC-15.

So, I believe we won't have any more new features until GCC-14 release.

Thanks.


juzhe.zhong@rivai.ai
 
From: Jeff Law
Date: 2024-01-05 23:50
To: Palmer Dabbelt; juzhe.zhong
CC: gcc-patches; Kito Cheng; Kito.cheng
Subject: Re: [committed] RISC-V: Add crypto vector builtin function.
 
 
On 1/4/24 20:24, Palmer Dabbelt wrote:
> On Thu, 04 Jan 2024 19:17:21 PST (-0800), juzhe.zhong@rivai.ai wrote:
>> Hi, Wang Feng.
>>
>> Your patch has some ICEs:
>> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (internal compiler 
>> error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc-intrinsic.c (test for excess errors)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (internal 
>> compiler error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-1.c (test for 
>> excess errors)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (internal 
>> compiler error: RTL check: expected code 'const_int', have 'reg' in 
>> vlmax_avl_type_p, at config/riscv/riscv-v.cc:4930)
>> FAIL: gcc.target/riscv/rvv/base/zvbc_vx_constraint-2.c (test for 
>> excess errors)
> 
> So let's just revert it, it doesn't even look like it was reviewed. 
> We've set a really bad precedent here where we're just merging a bunch 
> of unreviewed code and sorting out the regressions in trunk, that's not 
> the right way to do things.
> 
>>
>> I suspect you didn't enable rtl check in the regression:
>>
>> ../../configure --enable-gcc-checking=rtl.
>> Plz enable rtl check in the regression tests.
We haven't ever required folks to test with RTL checking enabled due to 
its compile-time cost.  So I don't think Feng did anything wrong here.
 
IIRC, Jakub's standard practice over in the x86 world is to do a 
bootstrap and regression test with RTL checking enabled in the spring as 
we get closer to the release to weed out these kinds of things that can 
slip through.
 
Clearly there's a bug and we should fix it, but it's not a sign that 
anything has gone terribly wrong.
 
jeff