public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jeff Law <jeffreyalaw@gmail.com>
To: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>,
	Jeff Law <jlaw@ventanamicro.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [committed] More gimple const/copy propagation opportunities
Date: Mon, 31 Oct 2022 16:43:42 -0600	[thread overview]
Message-ID: <235a80ee-97dc-2f78-e1b1-eb355ccde4fc@gmail.com> (raw)
In-Reply-To: <20221001205534.706b2024@nbbrfq>


On 10/1/22 12:55, Bernhard Reutner-Fischer wrote:
> On Fri, 30 Sep 2022 17:32:34 -0600
> Jeff Law <jlaw@ventanamicro.com> wrote:
>
>> +  /* This looks good from a CFG standpoint.  Now look at the guts
>> +     of PRED.  Basically we want to verify there are no PHI nodes
>> +     and no real statements.  */
>> +  if (! gimple_seq_empty_p (phi_nodes (pred)))
>> +    return false;
> So, given the below, neither DEBUG nor labels do count towards an
> empty seq [coming in from any PHI that is, otherwise it's a different
> thing], which is a bit surprising but well, ok. It looks at PHI IL, so
> probably yes. Allegedly that's what it is. Neat if that's true.

Right.  A forwarder block is allowed to have local labels, but not 
labels for nonlocal gotos, so we can and should ignore local labels.  
Debug statements must be ignored or you can end up with differing code 
generation based on whether or not -g is enabled or not.

In some contexts PHIs are allowed in forwarders, in other contexts they 
are not.  In this specific case I doubt it matters because of the 
restrictions we put on the CFG, the predecessor block is restricted to a 
single incoming edge.  The only way that'll have a PHI is if the PHI 
became a degenerate during DOM.




>
>> +
>> +  gimple_stmt_iterator gsi;
>> +  for (gsi = gsi_last_bb (pred); !gsi_end_p (gsi); gsi_prev (&gsi))
>> +    {
>> +      gimple *stmt = gsi_stmt (gsi);
>> +
>> +      switch (gimple_code (stmt))
>> +	{
>> +	  case GIMPLE_LABEL:
>> +	    if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
>> +	      return false;
>> +	    break;
>> +
>> +	  case GIMPLE_DEBUG:
>> +	    break;
>> +
>> +	  default:
>> +	    return false;
> don't like, sounds odd. Are we sure there's no other garbage that can
> manifest here? int meow=42;, and meow unused won't survive?, pragmas
> neither or stuff ?

That would generate a real statement and would thus be rejected. If 
there's anything other than a local label or debug statements in the 
block, then it's rejected.



>> @@ -583,6 +656,62 @@ record_edge_info (basic_block bb)
>>                 if (can_infer_simple_equiv && TREE_CODE (inverted) == EQ_EXPR)
>>   		edge_info->record_simple_equiv (op0, op1);
>>               }
>> +
>> +	  /* If this block is a single block loop, then we may be able to
>> +	     record some equivalences on the loop's exit edge.  */
>> +	  if (single_block_loop_p (bb))
>> +	    {
>> +	      /* We know it's a single block loop.  Now look at the loop
>> +		 exit condition.  What we're looking for is whether or not
>> +		 the exit condition is loop invariant which we can detect
>> +		 by checking if all the SSA_NAMEs referenced are defined
>> +		 outside the loop.  */
>> +	      if ((TREE_CODE (op0) != SSA_NAME
>> +		   || gimple_bb (SSA_NAME_DEF_STMT (op0)) != bb)
>> +		  && (TREE_CODE (op1) != SSA_NAME
>> +		      || gimple_bb (SSA_NAME_DEF_STMT (op1)) != bb))
>> +		{
>> +		  /* At this point we know the exit condition is loop
>> +		     invariant.  The only way to get out of the loop is
>> +		     if never traverses the backedge to begin with.  This
> s/if /if it /

Will fix.  THanks.


>
>> +		     implies that any PHI nodes create equivalances we can
> that any threw me off asking for "that if any". Would have been nicer,
> i think?

All PHIs at the target of the loop backedge in this case create an 
equivalence.  That's the whole point of the patch, to prove a set of 
circumstances that ultimately require all the PHIs on the loop backedge 
to create an equivalence on the loop exit.



>
>> +		     attach to the loop exit edge.  */
> attach it to

Updated, slightly differently, but should be clearer.


>
>> +		  int alternative
> bool

Sure.



>
>> +		    = (EDGE_PRED (bb, 0)->flags & EDGE_DFS_BACK) ? 1 : 0;
>> +
>> +		  gphi_iterator gsi;
>> +		  for (gsi = gsi_start_phis (bb);
>> +		       !gsi_end_p (gsi);
>> +		       gsi_next (&gsi))
>> +		    {
>> +		      /* If the other alternative is the same as the result,
>> +			 then this is a degenerate and can be ignored.  */
>> +		      if (dst == PHI_ARG_DEF (phi, !alternative))
>> +			continue;
>> +
>> +		      /* Now get the EDGE_INFO class so we can append
>> +			 it to our list.  We want the successor edge
>> +			 where the destination is not the source of
>> +			 an incoming edge.  */
>> +		      gphi *phi = gsi.phi ();
>> +		      tree src = PHI_ARG_DEF (phi, alternative);
>> +		      tree dst = PHI_RESULT (phi);
>> +
>> +		      if (EDGE_SUCC (bb, 0)->dest
>> +			  != EDGE_PRED (bb, !alternative)->src)
> by now, alternative would be easier to grok if it would have been spelled
> from_backedge_p or something. IMHO.

Agreed it's a bit ugly.  Let me think about  how to clean this up.


Anyway, I've queued the updates.  I'll push them the next time I do a 
bootstrap & regression test.


jeff



      reply	other threads:[~2022-10-31 22:43 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-30 23:32 Jeff Law
2022-10-01 18:55 ` Bernhard Reutner-Fischer
2022-10-31 22:43   ` Jeff Law [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=235a80ee-97dc-2f78-e1b1-eb355ccde4fc@gmail.com \
    --to=jeffreyalaw@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jlaw@ventanamicro.com \
    --cc=rep.dot.nop@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).