| Message ID | 20221224030800.221397-1-juzhe.zhong@rivai.ai |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Fix ICE of visiting non-existing block in CFG. | expand |
On 12/23/22 20:08, juzhe.zhong@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> > > This patch is to fix issue of visiting non-existing block of CFG. > Since blocks index of CFG in GCC are not always contiguous, we will potentially > visit a gap block which is no existing in the current CFG. > > This patch can avoid visiting non existing block in CFG. > > I noticed such issue in my internal regression of current testsuite > when I change the X86 server machine. This patch fix it: > 17:27:15 job(build_and_test_rv32): Increased FAIL List: > 17:27:15 job(build_and_test_rv32): FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-46.c > -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error: Segmentation fault) > > gcc/ChangeLog: > > * config/riscv/riscv-vsetvl.cc (pass_vsetvl::compute_global_backward_infos): Change to visit CFG. > (pass_vsetvl::prune_expressions): Ditto. The usual way to iterate over the blocks is something like this basic_block bb FOR_EACH_BB (bb, cfun) { do whatever you need on BB } Please use that form as that's what most folks working with GCC are already using. Jeff
On 12/27/22 16:11, juzhe.zhong wrote: > You mean only change to this form you suggested in this patch？ Since in > all other places of this PASS，I use RTL_SSA framework to iterate > instructions and blocks. I use RTL_SSA framework to iterate blocks here > to make codes look more consistent even though they are same here. The FOR_EACH_BB is used far more widely than the C++ style found in RTL-SSA so I'd slightly prefer that style. jeff
OK, I will change that after I finished my current work. juzhe.zhong@rivai.ai From: Jeff Law Date: 2022-12-28 08:06 To: juzhe.zhong CC: gcc-patches@gcc.gnu.org; kito.cheng@gmail.com; palmer@dabbelt.com Subject: Re: [PATCH] RISC-V: Fix ICE of visiting non-existing block in CFG. On 12/27/22 16:11, juzhe.zhong wrote: > You mean only change to this form you suggested in this patch？ Since in > all other places of this PASS，I use RTL_SSA framework to iterate > instructions and blocks. I use RTL_SSA framework to iterate blocks here > to make codes look more consistent even though they are same here. The FOR_EACH_BB is used far more widely than the C++ style found in RTL-SSA so I'd slightly prefer that style. jeff
On 12/27/22 17:24, 钟居哲 wrote:
> OK, I will change that after I finished my current work.
Sounds good. Thanks.
Jeff
oops, I just committed this yesterday, anyway, I think we can always have further patches to improve that. On Wed, Dec 28, 2022 at 9:12 AM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 12/27/22 17:24, 钟居哲 wrote: > > OK, I will change that after I finished my current work. > Sounds good. Thanks. > > Jeff
Hi, I fixed that form like you said: https://gcc.gnu.org/pipermail/gcc-patches/2022-December/609217.html juzhe.zhong@rivai.ai From: Jeff Law Date: 2022-12-28 09:11 To: 钟居哲 CC: gcc-patches; kito.cheng; palmer Subject: Re: [PATCH] RISC-V: Fix ICE of visiting non-existing block in CFG. On 12/27/22 17:24, 钟居哲 wrote: > OK, I will change that after I finished my current work. Sounds good. Thanks. Jeff
Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On 12/27/22 16:11, juzhe.zhong wrote: >> You mean only change to this form you suggested in this patch？ Since in >> all other places of this PASS，I use RTL_SSA framework to iterate >> instructions and blocks. I use RTL_SSA framework to iterate blocks here >> to make codes look more consistent even though they are same here. > The FOR_EACH_BB is used far more widely than the C++ style found in > RTL-SSA so I'd slightly prefer that style. I can see where you're coming from, but what the patch does is preferred for RTL-SSA passes. There is some additional information in rtl_ssa::bb_info compared to the underlying basic_block, and even if this particular loop doesn't use that information, IMO it would be better to avoid mixing styles within a pass. Also, the list that the patch iterates over is in reverse postorder, whereas FOR_EACH_BB doesn't guarantee a particular order. Again, that might not be important here, but it seems better to stick to the “native” RTL-SSA approach. Thanks, Richard
Yeah, I agree with you that it makes the pass looks confusing that if we are mixing FOR_EACH_BB and for (const bb_info *bb... But Jeff feels happy if I use FOR_EACH_BB so I send a patch to change the iterator form if it doesn't care about the order. In this patch, it's ok for both FOR_EACH_BB and for (const bb_info *bb... So I change it as Jeff suggested. However, in other places of this pass, for example compute_global_backward_infos function, I want to iterate blocks in reverse order and I must use "for (const bb_info *bb : crtl->ssa->reverse_bbs ())" which can allow me to do the information backward propagation throughly so that I can do the aggressive and fancy optimization. Base on these situations, it will be mixing FOR_EACH_BB and for (const bb_info *bb... in this pass which may make the pass a little bit confusing. juzhe.zhong@rivai.ai From: Richard Sandiford Date: 2022-12-28 19:47 To: Jeff Law via Gcc-patches CC: juzhe.zhong; Jeff Law; kito.cheng\@gmail.com; palmer\@dabbelt.com Subject: Re: [PATCH] RISC-V: Fix ICE of visiting non-existing block in CFG. Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On 12/27/22 16:11, juzhe.zhong wrote: >> You mean only change to this form you suggested in this patch？ Since in >> all other places of this PASS，I use RTL_SSA framework to iterate >> instructions and blocks. I use RTL_SSA framework to iterate blocks here >> to make codes look more consistent even though they are same here. > The FOR_EACH_BB is used far more widely than the C++ style found in > RTL-SSA so I'd slightly prefer that style. I can see where you're coming from, but what the patch does is preferred for RTL-SSA passes. There is some additional information in rtl_ssa::bb_info compared to the underlying basic_block, and even if this particular loop doesn't use that information, IMO it would be better to avoid mixing styles within a pass. Also, the list that the patch iterates over is in reverse postorder, whereas FOR_EACH_BB doesn't guarantee a particular order. Again, that might not be important here, but it seems better to stick to the “native” RTL-SSA approach. Thanks, Richard
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc index a55b5a1c394..0d66765e09c 100644 --- a/gcc/config/riscv/riscv-vsetvl.cc +++ b/gcc/config/riscv/riscv-vsetvl.cc @@ -1962,12 +1962,10 @@ pass_vsetvl::compute_global_backward_infos (void) if (dump_file) { fprintf (dump_file, "\n\nDirty blocks list: "); - for (size_t i = 0; i < m_vector_manager->vector_block_infos.length (); - i++) - { - if (m_vector_manager->vector_block_infos[i].reaching_out.dirty_p ()) - fprintf (dump_file, "%ld ", i); - } + for (const bb_info *bb : crtl->ssa->bbs ()) + if (m_vector_manager->vector_block_infos[bb->index ()] + .reaching_out.dirty_p ()) + fprintf (dump_file, "%d ", bb->index ()); fprintf (dump_file, "\n\n"); } } @@ -1976,15 +1974,16 @@ pass_vsetvl::compute_global_backward_infos (void) void pass_vsetvl::prune_expressions (void) { - for (size_t i = 0; i < m_vector_manager->vector_block_infos.length (); i++) + for (const bb_info *bb : crtl->ssa->bbs ()) { - if (m_vector_manager->vector_block_infos[i].local_dem.valid_or_dirty_p ()) + if (m_vector_manager->vector_block_infos[bb->index ()] + .local_dem.valid_or_dirty_p ()) m_vector_manager->create_expr ( - m_vector_manager->vector_block_infos[i].local_dem); - if (m_vector_manager->vector_block_infos[i] + m_vector_manager->vector_block_infos[bb->index ()].local_dem); + if (m_vector_manager->vector_block_infos[bb->index ()] .reaching_out.valid_or_dirty_p ()) m_vector_manager->create_expr ( - m_vector_manager->vector_block_infos[i].reaching_out); + m_vector_manager->vector_block_infos[bb->index ()].reaching_out); } if (dump_file)
From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> This patch is to fix issue of visiting non-existing block of CFG. Since blocks index of CFG in GCC are not always contiguous, we will potentially visit a gap block which is no existing in the current CFG. This patch can avoid visiting non existing block in CFG. I noticed such issue in my internal regression of current testsuite when I change the X86 server machine. This patch fix it: 17:27:15 job(build_and_test_rv32): Increased FAIL List: 17:27:15 job(build_and_test_rv32): FAIL: gcc.target/riscv/rvv/vsetvl/vlmax_back_prop-46.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error: Segmentation fault) gcc/ChangeLog: * config/riscv/riscv-vsetvl.cc (pass_vsetvl::compute_global_backward_infos): Change to visit CFG. (pass_vsetvl::prune_expressions): Ditto. --- gcc/config/riscv/riscv-vsetvl.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)