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
