* [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN
@ 2024-05-27 17:52 Hans-Peter Nilsson
2024-05-27 18:57 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: Hans-Peter Nilsson @ 2024-05-27 17:52 UTC (permalink / raw)
To: gcc-patches
Regtested cris-elf. Ok to commit?
-- >8 --
...and call compute_bb_for_insn in init_resource_info and
free_bb_for_insn in free_resource_info.
I put a gcc_unreachable in that else-clause for a failing
find_basic_block in mark_target_live_regs after the comment that says:
/* We didn't find the start of a basic block. Assume everything
in use. This should happen only extremely rarely. */
SET_HARD_REG_SET (res->regs);
and found that it fails not extremely rarely but extremely early in
the build (compiling libgcc).
That kind of pessimization leads to suboptimal delay-slot-filling.
Instead, do like many machine_dependent_reorg passes and call
compute_bb_for_insn as part of resource.cc initialization.
After this patch, there's a whole "if (b != -1)" conditional that's
dominated by a gcc_assert (b != -1). I separated that, as it's a NFC
whitespace patch that hampers patch readability.
Altogether this improved coremark performance for CRIS at -O2
-march=v10 by 0.36%.
* resource.cc: Include cfgrtl.h. Use BLOCK_FOR_INSN (insn)->index
instead of calling find_basic_block (insn). Assert for not -1.
(find_basic_block): Remove function.
(init_resource_info): Call compute_bb_for_insn.
(free_resource_info): Call free_bb_for_insn.
---
gcc/resource.cc | 66 ++++++++-----------------------------------------
1 file changed, 10 insertions(+), 56 deletions(-)
diff --git a/gcc/resource.cc b/gcc/resource.cc
index 06fcfd3e44c7..0d8cde93570e 100644
--- a/gcc/resource.cc
+++ b/gcc/resource.cc
@@ -28,6 +28,7 @@ along with GCC; see the file COPYING3. If not see
#include "tm_p.h"
#include "regs.h"
#include "emit-rtl.h"
+#include "cfgrtl.h"
#include "resource.h"
#include "insn-attr.h"
#include "function-abi.h"
@@ -75,7 +76,6 @@ static HARD_REG_SET current_live_regs;
static HARD_REG_SET pending_dead_regs;
\f
static void update_live_status (rtx, const_rtx, void *);
-static int find_basic_block (rtx_insn *, int);
static rtx_insn *next_insn_no_annul (rtx_insn *);
\f
/* Utility function called from mark_target_live_regs via note_stores.
@@ -113,46 +113,6 @@ update_live_status (rtx dest, const_rtx x, void *data ATTRIBUTE_UNUSED)
CLEAR_HARD_REG_BIT (pending_dead_regs, i);
}
}
-
-/* Find the number of the basic block with correct live register
- information that starts closest to INSN. Return -1 if we couldn't
- find such a basic block or the beginning is more than
- SEARCH_LIMIT instructions before INSN. Use SEARCH_LIMIT = -1 for
- an unlimited search.
-
- The delay slot filling code destroys the control-flow graph so,
- instead of finding the basic block containing INSN, we search
- backwards toward a BARRIER where the live register information is
- correct. */
-
-static int
-find_basic_block (rtx_insn *insn, int search_limit)
-{
- /* Scan backwards to the previous BARRIER. Then see if we can find a
- label that starts a basic block. Return the basic block number. */
- for (insn = prev_nonnote_insn (insn);
- insn && !BARRIER_P (insn) && search_limit != 0;
- insn = prev_nonnote_insn (insn), --search_limit)
- ;
-
- /* The closest BARRIER is too far away. */
- if (search_limit == 0)
- return -1;
-
- /* The start of the function. */
- else if (insn == 0)
- return ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb->index;
-
- /* See if any of the upcoming CODE_LABELs start a basic block. If we reach
- anything other than a CODE_LABEL or note, we can't find this code. */
- for (insn = next_nonnote_insn (insn);
- insn && LABEL_P (insn);
- insn = next_nonnote_insn (insn))
- if (BLOCK_FOR_INSN (insn))
- return BLOCK_FOR_INSN (insn)->index;
-
- return -1;
-}
\f
/* Similar to next_insn, but ignores insns in the delay slots of
an annulled branch. */
@@ -714,7 +674,8 @@ mark_target_live_regs (rtx_insn *insns, rtx target_maybe_return, struct resource
}
if (b == -1)
- b = find_basic_block (target, param_max_delay_slot_live_search);
+ b = BLOCK_FOR_INSN (target)->index;
+ gcc_assert (b != -1);
if (target_hash_table != NULL)
{
@@ -722,7 +683,7 @@ mark_target_live_regs (rtx_insn *insns, rtx target_maybe_return, struct resource
{
/* If the information is up-to-date, use it. Otherwise, we will
update it below. */
- if (b == tinfo->block && b != -1 && tinfo->bb_tick == bb_ticks[b])
+ if (b == tinfo->block && tinfo->bb_tick == bb_ticks[b])
{
res->regs = tinfo->live_regs;
return;
@@ -905,7 +866,6 @@ void
init_resource_info (rtx_insn *epilogue_insn)
{
int i;
- basic_block bb;
/* Indicate what resources are required to be valid at the end of the current
function. The condition code never is and memory always is.
@@ -975,10 +935,8 @@ init_resource_info (rtx_insn *epilogue_insn)
target_hash_table = XCNEWVEC (struct target_info *, TARGET_HASH_PRIME);
bb_ticks = XCNEWVEC (int, last_basic_block_for_fn (cfun));
- /* Set the BLOCK_FOR_INSN of each label that starts a basic block. */
- FOR_EACH_BB_FN (bb, cfun)
- if (LABEL_P (BB_HEAD (bb)))
- BLOCK_FOR_INSN (BB_HEAD (bb)) = bb;
+ /* Set the BLOCK_FOR_INSN for each insn. */
+ compute_bb_for_insn ();
}
\f
/* Free up the resources allocated to mark_target_live_regs (). This
@@ -987,8 +945,6 @@ init_resource_info (rtx_insn *epilogue_insn)
void
free_resource_info (void)
{
- basic_block bb;
-
if (target_hash_table != NULL)
{
int i;
@@ -1015,9 +971,7 @@ free_resource_info (void)
bb_ticks = NULL;
}
- FOR_EACH_BB_FN (bb, cfun)
- if (LABEL_P (BB_HEAD (bb)))
- BLOCK_FOR_INSN (BB_HEAD (bb)) = NULL;
+ free_bb_for_insn ();
}
\f
/* Clear any hashed information that we have stored for INSN. */
@@ -1063,10 +1017,10 @@ clear_hashed_info_until_next_barrier (rtx_insn *insn)
void
incr_ticks_for_insn (rtx_insn *insn)
{
- int b = find_basic_block (insn, param_max_delay_slot_live_search);
+ int b = BLOCK_FOR_INSN (insn)->index;
+ gcc_assert (b != -1);
- if (b != -1)
- bb_ticks[b]++;
+ bb_ticks[b]++;
}
\f
/* Add TRIAL to the set of resources used at the end of the current
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN
2024-05-27 17:52 [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN Hans-Peter Nilsson
@ 2024-05-27 18:57 ` Jeff Law
2024-05-28 20:18 ` Hans-Peter Nilsson
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Law @ 2024-05-27 18:57 UTC (permalink / raw)
To: Hans-Peter Nilsson, gcc-patches
On 5/27/24 11:52 AM, Hans-Peter Nilsson wrote:
> Regtested cris-elf. Ok to commit?
>
> -- >8 --
> ...and call compute_bb_for_insn in init_resource_info and
> free_bb_for_insn in free_resource_info.
>
> I put a gcc_unreachable in that else-clause for a failing
> find_basic_block in mark_target_live_regs after the comment that says:
>
> /* We didn't find the start of a basic block. Assume everything
> in use. This should happen only extremely rarely. */
> SET_HARD_REG_SET (res->regs);
>
> and found that it fails not extremely rarely but extremely early in
> the build (compiling libgcc).
>
> That kind of pessimization leads to suboptimal delay-slot-filling.
> Instead, do like many machine_dependent_reorg passes and call
> compute_bb_for_insn as part of resource.cc initialization.
>
> After this patch, there's a whole "if (b != -1)" conditional that's
> dominated by a gcc_assert (b != -1). I separated that, as it's a NFC
> whitespace patch that hampers patch readability.
>
> Altogether this improved coremark performance for CRIS at -O2
> -march=v10 by 0.36%.
>
> * resource.cc: Include cfgrtl.h. Use BLOCK_FOR_INSN (insn)->index
> instead of calling find_basic_block (insn). Assert for not -1.
> (find_basic_block): Remove function.
> (init_resource_info): Call compute_bb_for_insn.
> (free_resource_info): Call free_bb_for_insn.
I'm pretty sure that code as part of the overall problem -- namely that
we didn't have good basic block info so we resorted to insn scanning.
Presumably we set BLOCK_FOR_INSN when we generate a wrapper SEQUENCE
insns for a filled delay slot? Assuming we do create the right mapping
for those new insns, then this is OK.
jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN
2024-05-27 18:57 ` Jeff Law
@ 2024-05-28 20:18 ` Hans-Peter Nilsson
2024-05-31 9:12 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Hans-Peter Nilsson @ 2024-05-28 20:18 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
> Date: Mon, 27 May 2024 12:57:53 -0600
> From: Jeff Law <jeffreyalaw@gmail.com>
> > * resource.cc: Include cfgrtl.h. Use BLOCK_FOR_INSN (insn)->index
> > instead of calling find_basic_block (insn). Assert for not -1.
> > (find_basic_block): Remove function.
> > (init_resource_info): Call compute_bb_for_insn.
> > (free_resource_info): Call free_bb_for_insn.
> I'm pretty sure that code as part of the overall problem -- namely that
> we didn't have good basic block info so we resorted to insn scanning.
>
> Presumably we set BLOCK_FOR_INSN when we generate a wrapper SEQUENCE
> insns for a filled delay slot?
Yes - one way or the other: most insn chain changes from
reorg are through calls to add_insn_after, which always sets
the bb of the added insn according to the reference insn
(except when either insn is a barrier, then it never sets a
bb); see for example emit_delay_sequence. Others by
emit_insn_before and emit_copy_of_insn_after.
(Not-so-)fun fact: add_insn_after takes a bb parameter which
reorg.cc always passes as NULL. But - the argument is
*always ignored* and the bb in the "after" insn is used.
I traced that ignored parameter as far as
r0-81421-g6fb5fa3cbc0d78 "Merge dataflow branch into
mainline" when is was added. I *guess* it's an artifact
left over from some idea explored on that branch. Ripe for
obvious cleanup by removal everywhere.
> Assuming we do create the right mapping
> for those new insns, then this is OK.
Thanks for the quick review of the whole set!
brgds, H-P
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN
2024-05-28 20:18 ` Hans-Peter Nilsson
@ 2024-05-31 9:12 ` Richard Sandiford
0 siblings, 0 replies; 4+ messages in thread
From: Richard Sandiford @ 2024-05-31 9:12 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: Jeff Law, gcc-patches
Hans-Peter Nilsson <hp@axis.com> writes:
> [...]
> (Not-so-)fun fact: add_insn_after takes a bb parameter which
> reorg.cc always passes as NULL. But - the argument is
> *always ignored* and the bb in the "after" insn is used.
> I traced that ignored parameter as far as
> r0-81421-g6fb5fa3cbc0d78 "Merge dataflow branch into
> mainline" when is was added. I *guess* it's an artifact
> left over from some idea explored on that branch. Ripe for
> obvious cleanup by removal everywhere.
Heh. I wondered whether there'd be some direct callers of
add_insn_after_nobb that relied on the block *not* being updated
for some reason, but thankfully not. The only two callers seem
to be add_insn_after and emit_note_after. But then emit_note_after
handles notes slightly differently from add_insn_after, even though
logically, emitting an existing note should work in the same way
as emitting a new note.
So yeah, like you say, ripe for cleanup :)
Richard
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-31 9:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-27 17:52 [PATCH 2/4] resource.cc: Replace calls to find_basic_block with cfgrtl BLOCK_FOR_INSN Hans-Peter Nilsson
2024-05-27 18:57 ` Jeff Law
2024-05-28 20:18 ` Hans-Peter Nilsson
2024-05-31 9:12 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).