Re: [PATCH V2] Clean up loop-closed PHIs after loop finalize

[Jiufu Guo <guojiufu@linux.ibm.com>]

Richard Biener <rguenther@suse.de> writes:

> On Wed, 11 Nov 2020, Jiufu Guo wrote:
>
>> 
>> Thanks a lot for the sugguestion from previous mails.
>> The patch was updated accordingly.
>> 
>> This updated patch propagates loop-closed PHIs them out after
>> loop_optimizer_finalize under a new introduced flag.  At some cases,
>> to clean up loop-closed PHIs would save efforts of optimization passes
>> after loopdone.
>> 
>> This patch passes bootstrap and regtest on ppc64le.  Is this ok for trunk?
>
> Comments below
>
>>    free_numbers_of_iterations_estimates (fn);
>>  
>> +  if (flag_clean_up_loop_closed_phi
>
> Sorry if there was miscommunication but I've not meant to add a
> new user-visible flag but instead a flag argument to loop_optimizer_finalize
> (as said, you can default it to false to only need to change the
> one in fini_loops)
Sorry for misunderstand.
Updated the patch.
>
>> +      && loops_state_satisfies_p (fn, LOOP_CLOSED_SSA))
>> +    {
>> +      clean_up_loop_closed_phi (fn);
>> +      loops_state_clear (fn, LOOP_CLOSED_SSA);
>> +    }
>> +
......
>> +  gphi *phi;
>> +  tree rhs;
>> +  tree lhs;
>> +  gphi_iterator gsi;
>> +  struct loop *loop;
>> +  bool cfg_altered = false;
>> +
>> +  /* Check dominator info before get loop-close PHIs from loop exits.  */
>> +  if (dom_info_state (CDI_DOMINATORS) != DOM_OK)
>
> Why?
>
As you said, loop_optimizer_finalize is also called from where dominator
info is not ready, e.g. called from: vrp(execute_vrp).
At there, loop exits info is not ready, and then
get_loop_exit_edges/get_loop_body_with_size function does not work.

>> +  /* Walk over loop in function.  */
>> +  FOR_EACH_LOOP_FN (fun, loop, 0)
>> +    {
>> +      /* Check each exit edege of loop.  */
>> +      auto_vec<edge> exits = get_loop_exit_edges (loop);
>> +      FOR_EACH_VEC_ELT (exits, i, e)
>> +	if (single_pred_p (e->dest))
>> +	  /* Walk over loop-closed PHIs.  */
>> +	  for (gsi = gsi_start_phis (e->dest); !gsi_end_p (gsi);)
>> +	    {
>> +	      phi = gsi.phi ();
>> +	      rhs = degenerate_phi_result (phi);
>
> You have a single predecessor, thus the result is always degenerate.
>
>> +	      lhs = gimple_phi_result (phi);
>> +
>> +	      if (rhs && may_propagate_copy (lhs, rhs))
>> +		{
>> +		  gimple_stmt_iterator psi = gsi;
>> +		  /* Advance the iterator before stmt is removed.  */
>> +		  gsi_next (&gsi);
>
> remove_phi_node should take care of this, you shouldn't need this
> (just do not advance the iterator when you remove the PHI node).
>
Yeap, get it! Thanks, will update the patch accordingly.
>> +
......
>> +
>> +		  replace_uses_by (lhs, rhs);
>> +		  remove_phi_node (&psi, true);
>> +		  cfg_altered = true;
>
> in the end the return value is unused but I think we should avoid
> altering the CFG since doing so requires it to be cleaned up for
> unreachable blocks.  That means to open-code replace_uses_by as
>
>   imm_use_iterator imm_iter;
>   use_operand_p use;
>   gimple *stmt;
>   FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
>     {
>       FOR_EACH_IMM_USE_ON_STMT (use, imm_iter)
>         replace_exp (use, val);
>       update_stmt (stmt);
>     }

Thansk! This could also save some code in replace_uses_by.

BR. 
Jiufu Guo
>
> Thanks,
> Richard.
>
>> +		}
>> +	      else
>> +		gsi_next (&gsi);
>> +	    }
>> +    }
>> +
>> +  return cfg_altered;
>> +}
>>