RE: [PATCH RFA] rtl-optimization/PR50663, conditional propagation missed in cprop.c pass

["Bin Cheng" <bin dot cheng at arm dot com>]

Hi,
Thanks for your review.
Here comes the 2nd version patch modified according to your comments. Is it
ok?
Also could you please commit it if ok because I have no write access?

The new patch is tested against x86-linux-gnu.

Thanks.

2011-11-15  Bin Cheng  <bin.cheng@arm.com>

	PR rtl-optimization/50663
	* cprop.c (implicit_set_indexes): New global variable.
	(insert_set_in_table): Add additional parameter, record implicit set
info.
	(hash_scan_set): Add additional parameter.
	(compute_hash_table_work): And
	(hash_scan_insn): Pass implicit to hash_scan_set.
	(compute_cprop_data): Add implicit set to AVIN of block which the
	implicit set is recorded for.
	(one_cprop_pass): Handle implicit_set_indexes array.

> -----Original Message-----
> From: Eric Botcazou [mailto:ebotcazou@adacore.com]
> Sent: Saturday, November 12, 2011 12:35 AM
> To: Bin Cheng
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH RFA] rtl-optimization/PR50663, conditional propagation
> missed in cprop.c pass
> 
> > 2011-11-07  Bin Cheng  <bin.cheng@arm.com>
> >
> > 	PR rtl-optimization/50663
> > 	* cprop.c (bb_implicit): New global variable.
> > 	(insert_set_in_table): Add additional parameter, record implicit set
> > 	info.
> > 	(hash_scan_set): Add additional parameter.
> > 	(compute_hash_table_work): And
> > 	(hash_scan_insn): Pass implicit to hash_scan_set.
> > 	(compute_cprop_data): Add implicit set to AVIN of block which the
> >	implicit set is recorded for.
> > 	(one_cprop_pass): Handle bb_implicit array.
> 
> [80 columns at most for ChangeLog entries as well]
> 
> The patch is OK with the following changes:
> 
> @@ -116,6 +116,10 @@
>  /* Array of implicit set patterns indexed by basic block index.  */
>  static rtx *implicit_sets;
> 
> +/* Array of bitmap_index of corresponding implicit set, indexed by
> +   basic block index.  */
> +static int *bb_implicit;
> 
> A better name is implicit_set_indexes:
> 
> /* Array of indexes of expressions for implicit set patterns indexed by
basic
>    block index.  In other words, implicit_set_indexes[i] is the
bitmap_index
>    of the expression whose RTX is implicit_sets[i].  */
> static int *implicit_set_indexes;
> 
> 
> +  /* Record bitmap_index of the implicit set in bb_implicit.  */
> +  if (implicit)
> +    bb_implicit[BLOCK_FOR_INSN(cur_occr->insn)->index] =
> +      cur_expr->bitmap_index;
> 
> cur_occr->insn is just insn.
> 
> 
> +  /* Merge implicit set into CPROP_AVIN. There are always
> +     available at the entry of corresponding basic block.  */
> 
> "...implicit sets into CPROP_AVIN.  They are..."
> 
> +  FOR_EACH_BB (bb)
> +    {
> +      int index = bb_implicit[bb->index];
> +      if (index != -1)
> +	SET_BIT (cprop_avin[bb->index], (unsigned int)index);
> 
> The cast is superfluous.
> 
> I think that an explanation as to why we need to do this is in order
(after
> all, this went unnoticed until now) along the lines of: "We need to do
this
> because 1) implicit sets aren't recorded for the local pass so they cannot
> be propagated within their basic block by this pass and 2) the global pass
> would otherwise propagate them only in the successors of their basic
block."
> 
> Btw, you'll need to slightly adjust the patch because of my changes to
cprop.c.
> 
> Thanks for investigating and addressing this issue.
> 
> --
> Eric Botcazou

Attachment: pr50663-20111116.patch
Description: Binary data