Fix alignment propagation

> On Fri, Feb 20, 2015 at 12:39:29AM +0100, Jan Hubicka wrote:
> > Hi,
> > this patch fixes alignment propagation that causes wrong code on solex and firefox.
> > Patch is by Martin, I just added the obvous MINUS_EXPR fix (the offset would be wrong,
> > but I see no reason for MINUX_ExPR appearing there with constant parameter), went
> > ahead and commited the fix.
> > 
> > Tested on x86_64-linux.
> 
> Two nits:
> 
> > Index: ChangeLog
> > ===================================================================
> > --- ChangeLog	(revision 220825)
> > +++ ChangeLog	(working copy)
> > @@ -1,3 +1,10 @@
> > +2015-02-19  Martin Jambor  <mjmabor@suse.cz>
> 
> Typo in the e-mail address.
> 
> 
> Two ;'s at the end of the line.

Oops, looks like celebrations of lunar new year was bit too devastating.
This is followup patch that also fixes ;; issue.
It makes the alignment propagation to do proper lattice operations instead
of requiring perfect match and dropping everything to bottom otherwise.
Martin, does it look OK?

	* ipa-cp.c (ipcp_verify_propagated_values): Verify that there
	are no bottoms in alignment.
	(decrease_alignment, increase_alignment): New functions.
	(propagate_alignment_accross_jump_function): Use proper lattice
	operations
	* ipa-cp-1.c: New testcase.
	* ipa-cp-2.c: New testcase.

Hi,

On Fri, Feb 20, 2015 at 06:16:47AM +0100, Jan Hubicka wrote:
>
> ...
> 
> This is followup patch that also fixes ;; issue.
> It makes the alignment propagation to do proper lattice operations instead
> of requiring perfect match and dropping everything to bottom otherwise.
> Martin, does it look OK?

as I wrote in bugzilla, it is certainly an improvement which I like
but I suspect there are two bugs (and a small mistake in changelog):

> 
> 	* ipa-cp.c (ipcp_verify_propagated_values): Verify that there
> 	are no bottoms in alignment.

no TOPs

> 	(decrease_alignment, increase_alignment): New functions.
> 	(propagate_alignment_accross_jump_function): Use proper lattice
> 	operations
> 	* ipa-cp-1.c: New testcase.
> 	* ipa-cp-2.c: New testcase.
> Index: ipa-cp.c
> ===================================================================
> --- ipa-cp.c	(revision 220826)
> +++ ipa-cp.c	(working copy)
> @@ -1041,10 +1041,14 @@ ipcp_verify_propagated_values (void)
>        for (i = 0; i < count; i++)
>  	{
>  	  ipcp_lattice<tree> *lat = ipa_get_scalar_lat (info, i);
> +	  struct ipcp_param_lattices *plats;
> +	  plats = ipa_get_parm_lattices (info, i);
>  
> -	  if (!lat->bottom
> -	      && !lat->contains_variable
> -	      && lat->values_count == 0)
> +	  if ((!lat->bottom
> +	       && !lat->contains_variable
> +	       && lat->values_count == 0)
> +	      || (!plats->alignment.known
> +		  && opt_for_fn (node->decl, flag_ipa_cp_alignment)))
>  	    {
>  	      if (dump_file)
>  		{
> @@ -1409,6 +1413,60 @@ propagate_context_accross_jump_function
>    return ret;
>  }
>  
> +/* Decrease alignment info DEST to be at most CUR.  */
> +
> +static bool
> +decrease_alignment (ipa_alignment *dest, ipa_alignment cur)
> +{
> +  bool changed = false;
> +
> +  if (!cur.known)
> +    return false;

I really think this should be return set_alignment_to_bottom (dest);

If some known alignment has been already propagated to DEST along a
different edge and now along the current edge an unknown alignment is
coming in, then the result value of the lattice must be BOTTOM and not
the previous alignment this code leaves in place.

> +  if (!dest->known)
> +    {
> +      *dest = cur;
> +      return true;
> +    }
> +  if (dest->align == cur.align
> +      && dest->misalign == cur.misalign)
> +    return false;
> +
> +  if (dest->align > cur.align)
> +    {
> +      dest->align = cur.align;
> +      if (cur.align)
> +	dest->misalign
> +	  = dest->misalign % cur.align;
> +      changed = true;
> +    }
> +  if (dest->align && (dest->misalign != (cur.misalign % dest->align)))
> +    {
> +      int diff = abs (dest->misalign
> +		      - (cur.misalign % dest->align));
> +      dest->align = MIN (dest->align, (unsigned)diff & - diff);
> +      if (dest->align)
> +	dest->misalign
> +	  = dest->misalign % dest->align;
> +      changed = true;
> +    }
> +  return changed;
> +}
> +
> +/* Increase alignment info DEST to be at least CUR.  */
> +
> +static bool
> +increase_alignment (ipa_alignment *dest, ipa_alignment cur)
> +{
> +  if (!dest->known)
> +    return false;

I think this condition always exits.  DEST is caller's CUR which was
read from jfunc->alignment which is always going to be unknown for
PASS_THROUGH and ANCESTOR jump functions at the moment.  Perhaps you
meant if (!cur.known) ?

> +  if (!cur.known || dest->align < cur.align)
> +    {

again here, I think you meant !des->.known.

Apart from that, I am fine with the patch.
Thanks,

Martin

> > +/* Decrease alignment info DEST to be at most CUR.  */
> > +
> > +static bool
> > +decrease_alignment (ipa_alignment *dest, ipa_alignment cur)
> > +{
> > +  bool changed = false;
> > +
> > +  if (!cur.known)
> > +    return false;
> 
> I really think this should be return set_alignment_to_bottom (dest);
> 
> If some known alignment has been already propagated to DEST along a
> different edge and now along the current edge an unknown alignment is
> coming in, then the result value of the lattice must be BOTTOM and not
> the previous alignment this code leaves in place.

Well, because this is an optimisitic propagation now, !cur.known means TOP
that is "as good alginment as you can thunk of".
You have one known alignment in DEST and TOP in other, result is TOP.
> > +/* Increase alignment info DEST to be at least CUR.  */
> > +
> > +static bool
> > +increase_alignment (ipa_alignment *dest, ipa_alignment cur)
> > +{
> > +  if (!dest->known)
> > +    return false;
> 
> I think this condition always exits.  DEST is caller's CUR which was
> read from jfunc->alignment which is always going to be unknown for
> PASS_THROUGH and ANCESTOR jump functions at the moment.  Perhaps you
> meant if (!cur.known) ?

Again, it is just lattice operation. If dest is TOP, there is no
way to get it better.

I assumed that jfunc->alignment.known is meaningful even for PASSTHROUGH
(for example for parameters passed by invisible reference) and I turn
all !jfunc->alignment.known to BOTTOM in the conditional at beggining of
propagate_alignment_accross_jump_function.
If we never have useful alginment info on passthrough and ancestor,
I suppose we can just drop increase_alignment for now and always
go with CUR.
> 
> > +  if (!cur.known || dest->align < cur.align)
> > +    {
> 
> again here, I think you meant !des->.known.

Well, if CUR is TOP, you want to return CUR :)

Honza
> 
> Apart from that, I am fine with the patch.
> Thanks,
> 
> Martin