On 9/7/2021 7:23 AM, Aldy Hernandez wrote:
On 9/7/21 2:59 PM, Richard Biener wrote:
On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez
<al...@redhat.com> wrote:
On 9/6/21 9:19 AM, Richard Biener wrote:
On Fri, Sep 3, 2021 at 3:59 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
This patch abstracts out a couple common idioms in the forward
threader that I found useful while navigating the code base.
Tested on x86-64 Linux.
OK?
gcc/ChangeLog:
* tree-ssa-threadedge.c (has_phis_p): New.
(forwarder_block_p): New.
(potentially_threadable_block): Call forwarder_block_p.
(jump_threader::thread_around_empty_blocks): Call
has_phis_p.
(jump_threader::thread_through_normal_block): Call
forwarder_block_p.
---
gcc/tree-ssa-threadedge.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index e57f6d3e39c..3db54a199fd 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -95,6 +95,21 @@ jump_threader::thread_through_all_blocks (bool
may_peel_loop_headers)
return m_registry->thread_through_all_blocks
(may_peel_loop_headers);
}
+static inline bool
+has_phis_p (basic_block bb)
+{
+ return !gsi_end_p (gsi_start_phis (bb));
gimple_seq_empty_p (phi_nodes (bb)) shoud be cheaper. Do virtual PHIs
count as PHIs for you?
I don't know. The goal was to abstract some common idioms without
changing existing behavior, but if my abstractions confuse other
readers, perhaps I should revert my patch.
FWIW, my initial motivation here was to merge the path profitability
code between the forward and backward threaders. It seems the forward
threader is more permissive than the backward threader, even though the
latter can thread more paths than it's allowed (per profitable_path_p).
+}
+
+/* Return TRUE for a forwarder block which is defined as having PHIs
+ but no instructions. */
+
+static bool
+forwarder_block_p (basic_block bb)
There exists a function with exactly the same signature in
cfgrtl.h, likewise
several similar implementations might exist elsewhere.
Ughh, that's definitely not good.
Your definition is also quite odd, not matching what one would expect
(the PHI requirement). The tree-cfgcleanup.c variant has
tree_forwarder_block_p which is explicit about this.
Btw, gsi_start_nondebug_bb does not ignore labels.
Would a name like empty_block_with_phis_p be more appropriate?
I think so. That said, my main concern ist the clash with the same
named function.
Agreed.
OK for trunk?
Aldy
p.patch
commit 77ac56456d5db150d6a71eaca918f19d2b478f82
Author: Aldy Hernandez <al...@redhat.com>
Date: Tue Sep 7 15:20:23 2021 +0200
Rename forwarder_block_p in treading code to empty_block_with_phis_p.
gcc/ChangeLog:
* tree-ssa-threadedge.c (forwarder_block_p): Rename to...
(empty_block_with_phis_p): ...this.
(potentially_threadable_block): Same.
(jump_threader::thread_through_normal_block): Same.
OK. I nearly called out a request for a name change.... Guess I should
have :-)
jeff
