* [Committed] RISC-V: Use safe_grow_cleared for vector info [PR111469]
2023-09-30 23:03 ` 钟居哲
@ 2023-09-30 23:13 ` Patrick O'Neill
2023-10-02 7:12 ` [PATCH] " Jakub Jelinek
1 sibling, 0 replies; 4+ messages in thread
From: Patrick O'Neill @ 2023-09-30 23:13 UTC (permalink / raw)
To: 钟居哲, gcc-patches
Cc: jakub, pinskia, Jeff Law, gnu-toolchain
[-- Attachment #1: Type: text/plain, Size: 3702 bytes --]
Committed. Thanks Juzhe!
I had to adjust the changelog's PR formatting to get the pre-commit
hooks to accept it.
Here's the committed patch:
From f446cf5d58568e406cc81f434a63b3045942e9a9 Mon Sep 17 00:00:00 2001
From: Patrick O'Neill <patrick@rivosinc.com>
Date: Sat, 30 Sep 2023 15:50:11 -0700
Subject: [PATCH] RISC-V: Use safe_grow_cleared for vector info [PR111649]
Resolves a riscv*-*-* bootstrap failure due to a newly-turned-on assert.
2023-09-30 Jakub Jelinek <jakub@redhat.com>
gcc/ChangeLog:
PR target/111649
* config/riscv/riscv-vsetvl.cc
(vector_infos_manager::vector_infos_manager):
Replace safe_grow with safe_grow_cleared.
---
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111649
Fix authored by Jakub Jelinek.
Tested for regressions using multilib riscv glibc rv32gcv, rv64gcv
---
gcc/config/riscv/riscv-vsetvl.cc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/config/riscv/riscv-vsetvl.cc
b/gcc/config/riscv/riscv-vsetvl.cc
index af8c31d873c..4b06d93e7f9 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -2417,8 +2417,8 @@ vector_infos_manager::vector_infos_manager ()
vector_antin = nullptr;
vector_antout = nullptr;
vector_earliest = nullptr;
- vector_insn_infos.safe_grow (get_max_uid ());
- vector_block_infos.safe_grow (last_basic_block_for_fn (cfun));
+ vector_insn_infos.safe_grow_cleared (get_max_uid ());
+ vector_block_infos.safe_grow_cleared (last_basic_block_for_fn (cfun));
if (!optimize)
{
basic_block cfg_bb;
--
2.34.1
On 9/30/23 16:03, 钟居哲 wrote:
> LGTM.
>
> ------------------------------------------------------------------------
> juzhe.zhong@rivai.ai
>
> *From:* Patrick O'Neill <mailto:patrick@rivosinc.com>
> *Date:* 2023-10-01 07:00
> *To:* gcc-patches <mailto:gcc-patches@gcc.gnu.org>; juzhe.zhong
> <mailto:juzhe.zhong@rivai.ai>
> *CC:* jakub <mailto:jakub@redhat.com>; pinskia
> <mailto:pinskia@gcc.gnu.org>; JeffreyALaw
> <mailto:JeffreyALaw@gmail.com>; gnu-toolchain
> <mailto:gnu-toolchain@rivosinc.com>; Patrick O'Neill
> <mailto:patrick@rivosinc.com>
> *Subject:* [PATCH] RISC-V: Use safe_grow_cleared for vector info
> [PR111469]
> Resolves a riscv*-*-* bootstrap failure due to a newly-turned-on
> assert.
> 2023-09-30 Jakub Jelinek <jakub@redhat.com>
> PR target/111649
> gcc/ChangeLog:
> * config/riscv/riscv-vsetvl.cc
> (vector_infos_manager::vector_infos_manager):
> Replace safe_grow with safe_grow_cleared.
> ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111649
> Fix authored by Jakub Jelinek.
> Tested for regressions using multilib riscv glibc rv32gcv, rv64gcv
> ---
> gcc/config/riscv/riscv-vsetvl.cc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc
> b/gcc/config/riscv/riscv-vsetvl.cc
> index af8c31d873c..4b06d93e7f9 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -2417,8 +2417,8 @@ vector_infos_manager::vector_infos_manager ()
> vector_antin = nullptr;
> vector_antout = nullptr;
> vector_earliest = nullptr;
> - vector_insn_infos.safe_grow (get_max_uid ());
> - vector_block_infos.safe_grow (last_basic_block_for_fn (cfun));
> + vector_insn_infos.safe_grow_cleared (get_max_uid ());
> + vector_block_infos.safe_grow_cleared (last_basic_block_for_fn
> (cfun));
> if (!optimize)
> {
> basic_block cfg_bb;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] RISC-V: Use safe_grow_cleared for vector info [PR111469]
2023-09-30 23:03 ` 钟居哲
2023-09-30 23:13 ` [Committed] " Patrick O'Neill
@ 2023-10-02 7:12 ` Jakub Jelinek
1 sibling, 0 replies; 4+ messages in thread
From: Jakub Jelinek @ 2023-10-02 7:12 UTC (permalink / raw)
To: 钟居哲, Jeff Law; +Cc: gcc-patches
On Sun, Oct 01, 2023 at 07:03:51AM +0800, 钟居哲 wrote:
> LGTM.
>
> juzhe.zhong@rivai.ai
>
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
> index af8c31d873c..4b06d93e7f9 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -2417,8 +2417,8 @@ vector_infos_manager::vector_infos_manager ()
> vector_antin = nullptr;
> vector_antout = nullptr;
> vector_earliest = nullptr;
> - vector_insn_infos.safe_grow (get_max_uid ());
> - vector_block_infos.safe_grow (last_basic_block_for_fn (cfun));
> + vector_insn_infos.safe_grow_cleared (get_max_uid ());
> + vector_block_infos.safe_grow_cleared (last_basic_block_for_fn (cfun));
> if (!optimize)
> {
> basic_block cfg_bb;
Note, while it works around the build failure, I strongly doubt it is the
right fix in this case. The other spots which have been changed similarly
are quite different, all the vec<bitmap_head> cases have been followed
(almost) immediately by bitmap_initialize of all the elements or just 1-3
elements and their actual uses.
The above is very different. Sizing a vector with get_max_uid ()
means it is very likely very sparse and so constructing every element in the
array seems undesirable. While it is true that e.g. IRA or DF use vectors
indexed by INSN_UID, I think the vectors pretty much always have pointer
elements and say pool allocate what it points to upon first access.
While vector_insn_info is huge (48 bytes on 64-bit hosts from what I can see)
even without much attempts to make it shorter (e.g. the vl_vtype_info member
ordering of 2xpointer sized member, 1 byte member, 4 byte member, 3 1 byte
members creates unnecessary padding).
The reason why arrays indexed by INSN_UID are sparse are 3:
1) we have --param min-nondebug-insn-uid= parameter (where, albeit usually
just for debugging, one can specify very high start for those, say
--param min-nondebug-insn-uid=100000 means debug insns will be created
with INSN_UID 1 and up, while non-DEBUG_INSNs only with INSN_UID 100000
and up, so even a function containing just 3-4 insns will have
get_max_uid () of 100004 or so; the above allocates in that case
100004 * 48 bytes (bad) and newly constructs all the elements in it
2) INSN_UIDs are given to all newly created insns during RTL optimizations,
when an insn is deleted, its INSN_UID is not reused, so there can be
large gaps; the more churn in RTL optimizations, the larger
3) INSN_UIDs are given even to BARRIERs, DEBUG_INSNs etc., plus the question
is if the algorithm really needs to track every "normal" insn as well,
whether it shouldn't track just those that are in some ways using vectors
or relevant to it, many scalar instructions might be uninteresting,
DEBUG_INSNs certainly should be uninteresting (they must not affect
code generation), etc.
So, I think having something lazily allocated and initialized on demand
might be a compile time memory and time win.
For basic blockswe perform some block number compactions during compilation,
so those shouldn't be denser. But the structure for each basic block is
even bigger (104 bytes, because it contains 2 x 48 plus probability).
E.g. DF uses for many purposes LUIDs instead, which need to be recomputed,
but are logical uids within each basic block.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread