Re: [PATCH 07/14] Covert ipa-pure-const.c to symbol_summary.

[Martin Liška <mliska at suse dot cz>]

On 06/07/2018 02:57 PM, Jan Hubicka wrote:
>>
>> gcc/ChangeLog:
>>
>> 2018-04-24  Martin Liska  <mliska@suse.cz>
>>
>> 	* ipa-pure-const.c (struct funct_state_d): Do it class instead
>> 	of struct.
>> 	(class funct_state_summary_t): New function_summary class.
>> 	(has_function_state): Remove.
>> 	(get_function_state): Likewise.
>> 	(set_function_state): Likewise.
>> 	(add_new_function): Likewise.
>> 	(funct_state_summary_t::insert): New function.
>> 	(duplicate_node_data): Remove.
>> 	(remove_node_data): Remove.
>> 	(funct_state_summary_t::duplicate): New function.
>> 	(register_hooks): Create new funct_state_summaries.
>> 	(pure_const_generate_summary): Use it.
>> 	(pure_const_write_summary): Likewise.
>> 	(pure_const_read_summary): Likewise.
>> 	(propagate_pure_const): Likewise.
>> 	(propagate_nothrow): Likewise.
>> 	(dump_malloc_lattice): Likewise.
>> 	(propagate_malloc): Likewise.
>> 	(execute): Do not register hooks, just remove summary
>> 	instead.
>> 	(pass_ipa_pure_const::pass_ipa_pure_const): Simplify
>> 	constructor.
> 
> OK with changes below.
> In general, it would be cool to reorg this pass into simple SCC propagation template
> because it does same things over and over again (sometimes on slightly different graph
> because it has feature to skip uninteresting edges e.g. for nothrow propagation).
>> @@ -1485,7 +1439,7 @@ propagate_pure_const (void)
>>  	  int i;
>>  	  struct ipa_ref *ref = NULL;
>>  
>> -	  funct_state w_l = get_function_state (w);
>> +	  funct_state w_l = funct_state_summaries->get_create (w);
>>  	  if (dump_file && (dump_flags & TDF_DETAILS))
>>  	    fprintf (dump_file, "  Visiting %s state:%s looping %i\n",
>>  		     w->dump_name (),
>> @@ -1527,7 +1481,7 @@ propagate_pure_const (void)
>>  		}
>>  	      if (avail > AVAIL_INTERPOSABLE)
>>  		{
>> -		  funct_state y_l = get_function_state (y);
>> +		  funct_state y_l = funct_state_summaries->get_create (y);
>>  		  if (dump_file && (dump_flags & TDF_DETAILS))
>>  		    {
>>  		      fprintf (dump_file,
> 
> The functions are organized in a way that it goes cycle by cycle. First loop initializes
> everything in the cycle and in this case you want get_create (where w_l is set)
> y_l looks for calls and because it works in SCC order all calls are either in same cycle
> or processed.  I do not see any guard checking that y_l is initialized and because you
> now initialize it to pesimistic state I think it will turn any SCC component into non-pure
> now (while it originaly worked by initializing to 0 which is optimistic state CONST)
> 
> So i would add guard that y is in different SCC component and make code to crash
> if get() returns NULL.
> Similarly for all other propagators.
> 
>> @@ -1642,7 +1596,7 @@ propagate_pure_const (void)
>>        while (w && !can_free)
>>  	{
>>  	  struct cgraph_edge *e;
>> -	  funct_state w_l = get_function_state (w);
>> +	  funct_state w_l = funct_state_summaries->get_create (w);
>>  
>>  	  if (w_l->can_free
>>  	      || w->get_availability () == AVAIL_INTERPOSABLE
>> @@ -1657,7 +1611,7 @@ propagate_pure_const (void)
>>  								  e->caller);
>>  
>>  	      if (avail > AVAIL_INTERPOSABLE)
>> -		can_free = get_function_state (y)->can_free;
>> +		can_free = funct_state_summaries->get_create (y)->can_free;
>>  	      else
>>  		can_free = true;
>>  	    }
> 
> Here everything should be computed already.
> 
> OK with those changes provides it does not affect generated code.
> 
> Honza
> 

Hi.

Ok, I'll do that with one additional patch that I'm sending.

Martin

>From 42f122845969bf39ec606a0592d7a511b87c4657 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 8 Jun 2018 12:59:34 +0200
Subject: [PATCH] Make ipa-pure-const more strict about summary constrains.

gcc/ChangeLog:

2018-06-08  Martin Liska  <mliska@suse.cz>

	* ipa-pure-const.c (propagate_pure_const): Use ::get at places
        where we expect an existing summary.
---
 gcc/ipa-pure-const.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 8c415bc1fc0..d0f9cb8d7f7 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -1477,7 +1477,7 @@ propagate_pure_const (void)
 		}
 	      if (avail > AVAIL_INTERPOSABLE)
 		{
-		  funct_state y_l = funct_state_summaries->get_create (y);
+		  funct_state y_l = funct_state_summaries->get (y);
 		  if (dump_file && (dump_flags & TDF_DETAILS))
 		    {
 		      fprintf (dump_file,
@@ -1591,7 +1591,7 @@ propagate_pure_const (void)
       while (w && !can_free)
 	{
 	  struct cgraph_edge *e;
-	  funct_state w_l = funct_state_summaries->get_create (w);
+	  funct_state w_l = funct_state_summaries->get (w);
 
 	  if (w_l->can_free
 	      || w->get_availability () == AVAIL_INTERPOSABLE
@@ -1606,7 +1606,7 @@ propagate_pure_const (void)
 								  e->caller);
 
 	      if (avail > AVAIL_INTERPOSABLE)
-		can_free = funct_state_summaries->get_create (y)->can_free;
+		can_free = funct_state_summaries->get (y)->can_free;
 	      else
 		can_free = true;
 	    }
@@ -1619,7 +1619,7 @@ propagate_pure_const (void)
       w = node;
       while (w)
 	{
-	  funct_state w_l = funct_state_summaries->get_create (w);
+	  funct_state w_l = funct_state_summaries->get (w);
 	  enum pure_const_state_e this_state = pure_const_state;
 	  bool this_looping = looping;
 
-- 
2.17.0