Re: [PATCH] combine: If a parallel I2 was split, do not allow a new I2 (PR64268)

[Paolo Bonzini <bonzini at gnu dot org>]

On 15/12/2014 12:05, Segher Boessenkool wrote:
> If combine is given a parallel I2, it splits it into separate I1 and
> I2 insns in some cases (one case since the beginning of time; more
> cases since my r218248).  It gives the new I1 the same UID as the I2
> already has; there is a comment that this works fine because the I1
> never is added to the insn stream.
> 
> When combine eventually replaces the insns with something new, it
> calls SET_INSN_DELETED on those it wants gone.  Since r125624 (when
> DF was added, back in 2007) SET_INSN_DELETED uses the UID of the insn
> it is called for to do the deletion for dataflow.  So since then, when
> such a split I1 is deleted, DF thinks I2 is deleted as well.  This
> of course explodes if I2 is in fact needed (but only if that I2 still
> exists at the end of combine, i.e. the insn was not combined to further
> down; that might explain why it wasn't noticed before).
> 
> This should be fixed properly, but it is somewhat involved.  This
> patch simply disallows all combinations coming from a split parallel
> I2 if it needs a new I2.  This fixes PR target/64268.
> 
> Bootstrapped on powerpc64-linux and powerpc-linux; the fails are gone.
> Regtested on powerpc64-linux -m32,-m32/-mpowerpc64,-m64,-m64/-mlra.
> Is this okay for mainline?
> 
> Segher
> 
> 
> 2014-12-15  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> gcc/
> 	* combine.c (try_combine): Don't try to split newpat if we started
> 	with I2 a PARALLEL that we split.
> 
> ---
>  gcc/combine.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 8995c1d3..de2e49f 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -3471,6 +3471,17 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
>    if (insn_code_number < 0)
>      insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);
>  
> +  /* If we got I1 and I2 from splitting the original (parallel) I2 into two,
> +     I1 and I2 have the same UID, which means that DF ends up deleting I2
> +     when it is asked to delete I1.  So only allow this if we want I2 deleted,
> +     that is, if we get only one insn as combine result; don't try to split
> +     off a new I2 if it doesn't match yet.  */
> +  if (i1 && insn_code_number < 0 && INSN_UID (i1) == INSN_UID (i2))
> +    {
> +      undo_all ();
> +      return 0;
> +    }
> +
>    /* If we were combining three insns and the result is a simple SET
>       with no ASM_OPERANDS that wasn't recognized, try to split it into two
>       insns.  There are two ways to do this.  It can be split using a
> 

Random questions:

1) did you check that it never triggers on e.g. an x86 bootstrap, and
that it doesn't trigger too often on PPC64?

2) if it triggers rarely, should combine just try and give a new UID to
i1?  What makes that hard?  Or should it just skip the SET_INSN_DELETED
on i1 unless it is added to the instruction stream?

That said, if (1) is true, this looks like this fix is good enough for 5
and open branches.

Thanks David for pointing this out to me.

Paolo