[PATCH] Improve backwards threader debugging dumps.

[PATCH] Improve backwards threader debugging dumps.

[Aldy Hernandez]

This patch adds debugging helpers to the backwards threader.  I have
also noticed that profitable_path_p() can bail early on paths that
crosses loops and leave the dump of blocks incomplete.  Fixed as
well.

Unfortunately the new methods cannot be marked const, because we call
the solver's dump which is not const.  I believe this was because the
ranger dump calls m_cache.block_range().  This could probably use a
cleanup at a later time.

Tested on x86-64 Linux.

OK?

gcc/ChangeLog:

	* tree-ssa-threadbackward.c (back_threader::dump): New.
	(back_threader::debug): New.
	(back_threader_profitability::profitable_path_p): Dump blocks
	even if we are bailing early.
---
 gcc/tree-ssa-threadbackward.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gcc/tree-ssa-threadbackward.c b/gcc/tree-ssa-threadbackward.c
index 3aad1493c4d..b9a0d9a60ad 100644
--- a/gcc/tree-ssa-threadbackward.c
+++ b/gcc/tree-ssa-threadbackward.c
@@ -42,6 +42,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-range-path.h"
 #include "ssa.h"
 #include "tree-cfgcleanup.h"
+#include "tree-pretty-print.h"
 
 // Path registry for the backwards threader.  After all paths have been
 // registered with register_path(), thread_through_all_blocks() is called
@@ -89,6 +90,8 @@ private:
   edge find_taken_edge (const vec<basic_block> &path);
   edge find_taken_edge_cond (const vec<basic_block> &path, gcond *);
   edge find_taken_edge_switch (const vec<basic_block> &path, gswitch *);
+  virtual void debug ();
+  virtual void dump (FILE *out);
 
   back_threader_registry m_registry;
   back_threader_profitability m_profit;
@@ -519,6 +522,30 @@ debug (const vec <basic_block> &path)
   dump_path (stderr, path);
 }
 
+void
+back_threader::dump (FILE *out)
+{
+  m_solver.dump (out);
+  fprintf (out, "\nCandidates for pre-computation:\n");
+  fprintf (out, "===================================\n");
+
+  bitmap_iterator bi;
+  unsigned i;
+
+  EXECUTE_IF_SET_IN_BITMAP (m_imports, 0, i, bi)
+    {
+      tree name = ssa_name (i);
+      print_generic_expr (out, name, TDF_NONE);
+      fprintf (out, "\n");
+    }
+}
+
+void
+back_threader::debug ()
+{
+  dump (stderr);
+}
+
 back_threader_registry::back_threader_registry (int max_allowable_paths)
   : m_max_allowable_paths (max_allowable_paths)
 {
@@ -607,6 +634,14 @@ back_threader_profitability::profitable_path_p (const vec<basic_block> &m_path,
 	  if (bb->loop_father != loop)
 	    {
 	      path_crosses_loops = true;
+
+	      // Dump rest of blocks.
+	      if (dump_file && (dump_flags & TDF_DETAILS))
+		for (j++; j < m_path.length (); j++)
+		  {
+		    bb = m_path[j];
+		    fprintf (dump_file, " bb:%i", bb->index);
+		  }
 	      break;
 	    }
 
-- 
2.31.1

[PATCH] Abstract PHI and forwarder block checks in jump threader.

[Aldy Hernandez]

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));
+}
+
+/* Return TRUE for a forwarder block which is defined as having PHIs
+   but no instructions.  */
+
+static bool
+forwarder_block_p (basic_block bb)
+{
+  return gsi_end_p (gsi_start_nondebug_bb (bb)) && has_phis_p (bb);
+}
+
 /* Return TRUE if we may be able to thread an incoming edge into
    BB to an outgoing edge from BB.  Return FALSE otherwise.  */
 
@@ -107,9 +122,8 @@ potentially_threadable_block (basic_block bb)
      not optimized away because they forward from outside a loop
      to the loop header.   We want to thread through them as we can
      sometimes thread to the loop exit, which is obviously profitable.
-     the interesting case here is when the block has PHIs.  */
-  if (gsi_end_p (gsi_start_nondebug_bb (bb))
-      && !gsi_end_p (gsi_start_phis (bb)))
+     The interesting case here is when the block has PHIs.  */
+  if (forwarder_block_p (bb))
     return true;
 
   /* If BB has a single successor or a single predecessor, then
@@ -854,7 +868,7 @@ jump_threader::thread_around_empty_blocks (vec<jump_thread_edge *> *path,
   /* The key property of these blocks is that they need not be duplicated
      when threading.  Thus they cannot have visible side effects such
      as PHI nodes.  */
-  if (!gsi_end_p (gsi_start_phis (bb)))
+  if (has_phis_p (bb))
     return false;
 
   /* Skip over DEBUG statements at the start of the block.  */
@@ -994,8 +1008,7 @@ jump_threader::thread_through_normal_block (vec<jump_thread_edge *> *path,
     {
       /* First case.  The statement simply doesn't have any instructions, but
 	 does have PHIs.  */
-      if (gsi_end_p (gsi_start_nondebug_bb (e->dest))
-	  && !gsi_end_p (gsi_start_phis (e->dest)))
+      if (forwarder_block_p (e->dest))
 	return 0;
 
       /* Second case.  */
-- 
2.31.1

Re: [PATCH] Improve backwards threader debugging dumps.

[Jeff Law]

On 9/3/2021 7:56 AM, Aldy Hernandez wrote:
> This patch adds debugging helpers to the backwards threader.  I have
> also noticed that profitable_path_p() can bail early on paths that
> crosses loops and leave the dump of blocks incomplete.  Fixed as
> well.
>
> Unfortunately the new methods cannot be marked const, because we call
> the solver's dump which is not const.  I believe this was because the
> ranger dump calls m_cache.block_range().  This could probably use a
> cleanup at a later time.
>
> Tested on x86-64 Linux.
>
> OK?
>
> gcc/ChangeLog:
>
> 	* tree-ssa-threadbackward.c (back_threader::dump): New.
> 	(back_threader::debug): New.
> 	(back_threader_profitability::profitable_path_p): Dump blocks
> 	even if we are bailing early.
OK
jeff

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Jeff Law]

On 9/3/2021 7:56 AM, Aldy Hernandez 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.
OK
jeff

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Jeff Law]

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 
>> <aldyh@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 <aldyh@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

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Aldy Hernandez]

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



On 9/7/21 2:59 PM, Richard Biener wrote:
> On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez <aldyh@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


[-- Attachment #2: p.patch --]
[-- Type: text/x-patch, Size: 1818 bytes --]

commit 77ac56456d5db150d6a71eaca918f19d2b478f82
Author: Aldy Hernandez <aldyh@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.

diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 3db54a199fd..3c7cdc58b93 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -101,11 +101,10 @@ has_phis_p (basic_block bb)
   return !gsi_end_p (gsi_start_phis (bb));
 }
 
-/* Return TRUE for a forwarder block which is defined as having PHIs
-   but no instructions.  */
+/* Return TRUE for a block with PHIs but no statements.  */
 
 static bool
-forwarder_block_p (basic_block bb)
+empty_block_with_phis_p (basic_block bb)
 {
   return gsi_end_p (gsi_start_nondebug_bb (bb)) && has_phis_p (bb);
 }
@@ -123,7 +122,7 @@ potentially_threadable_block (basic_block bb)
      to the loop header.   We want to thread through them as we can
      sometimes thread to the loop exit, which is obviously profitable.
      The interesting case here is when the block has PHIs.  */
-  if (forwarder_block_p (bb))
+  if (empty_block_with_phis_p (bb))
     return true;
 
   /* If BB has a single successor or a single predecessor, then
@@ -1008,7 +1007,7 @@ jump_threader::thread_through_normal_block (vec<jump_thread_edge *> *path,
     {
       /* First case.  The statement simply doesn't have any instructions, but
 	 does have PHIs.  */
-      if (forwarder_block_p (e->dest))
+      if (empty_block_with_phis_p (e->dest))
 	return 0;
 
       /* Second case.  */

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Richard Biener]

On September 7, 2021 12:02:27 PM GMT+02:00, Aldy Hernandez <aldyh@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.

Richard. 

>Aldy
>

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Aldy Hernandez]

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?

Aldy

Re: [PATCH] Abstract PHI and forwarder block checks in jump threader.

[Richard Biener]

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?

> +}
> +
> +/* 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.

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.

> +{
> +  return gsi_end_p (gsi_start_nondebug_bb (bb)) && has_phis_p (bb);
> +}
> +
>  /* Return TRUE if we may be able to thread an incoming edge into
>     BB to an outgoing edge from BB.  Return FALSE otherwise.  */
>
> @@ -107,9 +122,8 @@ potentially_threadable_block (basic_block bb)
>       not optimized away because they forward from outside a loop
>       to the loop header.   We want to thread through them as we can
>       sometimes thread to the loop exit, which is obviously profitable.
> -     the interesting case here is when the block has PHIs.  */
> -  if (gsi_end_p (gsi_start_nondebug_bb (bb))
> -      && !gsi_end_p (gsi_start_phis (bb)))
> +     The interesting case here is when the block has PHIs.  */
> +  if (forwarder_block_p (bb))
>      return true;
>
>    /* If BB has a single successor or a single predecessor, then
> @@ -854,7 +868,7 @@ jump_threader::thread_around_empty_blocks (vec<jump_thread_edge *> *path,
>    /* The key property of these blocks is that they need not be duplicated
>       when threading.  Thus they cannot have visible side effects such
>       as PHI nodes.  */
> -  if (!gsi_end_p (gsi_start_phis (bb)))
> +  if (has_phis_p (bb))
>      return false;
>
>    /* Skip over DEBUG statements at the start of the block.  */
> @@ -994,8 +1008,7 @@ jump_threader::thread_through_normal_block (vec<jump_thread_edge *> *path,
>      {
>        /* First case.  The statement simply doesn't have any instructions, but
>          does have PHIs.  */
> -      if (gsi_end_p (gsi_start_nondebug_bb (e->dest))
> -         && !gsi_end_p (gsi_start_phis (e->dest)))
> +      if (forwarder_block_p (e->dest))
>         return 0;
>
>        /* Second case.  */
> --
> 2.31.1
>

[PATCH] Abstract PHI and forwarder block checks in jump threader.

[Aldy Hernandez]

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));
+}
+
+/* Return TRUE for a forwarder block which is defined as having PHIs
+   but no instructions.  */
+
+static bool
+forwarder_block_p (basic_block bb)
+{
+  return gsi_end_p (gsi_start_nondebug_bb (bb)) && has_phis_p (bb);
+}
+
 /* Return TRUE if we may be able to thread an incoming edge into
    BB to an outgoing edge from BB.  Return FALSE otherwise.  */
 
@@ -107,9 +122,8 @@ potentially_threadable_block (basic_block bb)
      not optimized away because they forward from outside a loop
      to the loop header.   We want to thread through them as we can
      sometimes thread to the loop exit, which is obviously profitable.
-     the interesting case here is when the block has PHIs.  */
-  if (gsi_end_p (gsi_start_nondebug_bb (bb))
-      && !gsi_end_p (gsi_start_phis (bb)))
+     The interesting case here is when the block has PHIs.  */
+  if (forwarder_block_p (bb))
     return true;
 
   /* If BB has a single successor or a single predecessor, then
@@ -854,7 +868,7 @@ jump_threader::thread_around_empty_blocks (vec<jump_thread_edge *> *path,
   /* The key property of these blocks is that they need not be duplicated
      when threading.  Thus they cannot have visible side effects such
      as PHI nodes.  */
-  if (!gsi_end_p (gsi_start_phis (bb)))
+  if (has_phis_p (bb))
     return false;
 
   /* Skip over DEBUG statements at the start of the block.  */
@@ -994,8 +1008,7 @@ jump_threader::thread_through_normal_block (vec<jump_thread_edge *> *path,
     {
       /* First case.  The statement simply doesn't have any instructions, but
 	 does have PHIs.  */
-      if (gsi_end_p (gsi_start_nondebug_bb (e->dest))
-	  && !gsi_end_p (gsi_start_phis (e->dest)))
+      if (forwarder_block_p (e->dest))
 	return 0;
 
       /* Second case.  */
-- 
2.31.1