Re: [PATCH] Fix PR 61225

[Jeff Law <law at redhat dot com>]

On 12/09/14 12:07, Segher Boessenkool wrote:
> On Tue, Dec 09, 2014 at 05:49:18PM +0800, Zhenqiang Chen wrote:
> > > Do you need to verify SETA and SETB satisfy single_set?  Or has that
> > > already been done elsewhere?
> >
> > A is NEXT_INSN (insn)
> > B is prev_nonnote_nondebug_insn (insn),
> >
> > For I1 -> I2 -> B; I2 -> A;
> > LOG_LINK can make sure I1 and I2 are single_set,
>
> It cannot, not anymore anyway.  LOG_LINKs can point to an insn with multiple
> SETs; multiple LOG_LINKs can point to such an insn.
So let's go ahead and put a single_set test in this function.

> > > > Is this fragment really needed?  Does it ever trigger?  I'd think that
> > > for > 2 uses punting would be fine.  Do we really commonly have cases
> > > with > 2 uses, but where they're all in SETA and SETB?
>
> Can't you just check for a death note on the second insn?  Together with
> reg_used_between_p?
Yea, that'd accomplish the same thing I think Zhenqiang is trying to 
catch and is simpler than walking the lists.

> > > > +	  /* Try to combine a compare insn that sets CC
> > > > +	     with a preceding insn that can set CC, and maybe with its
> > > > +	     logical predecessor as well.
> > > > +	     We need this special code because data flow connections
> > > > +	     do not get entered in LOG_LINKS.  */
>
> I think you mean "not _all_ data flow connections"?
I almost said something about this comment, but figured I was nitpicking 
too much :-)

> > > So you've got two new combine cases here, but I think the testcase only
> > > tests one of them.  Can you include a testcase for both of hte major
> > > paths above (I1->I2->I3; I2->insn and I2->I3; I2->INSN)
> >
> > pr61225.c is the case to cover I1->I2->I3; I2->insn.
> >
> > For I2 -> I3; I2 -> insn, I tried my test cases and found peephole2 can also
> > handle them. So I removed the code from the patch.
>
> Why?  The simpler case has much better chances of being used.
The question does it actually catch anything not already handled?  I 
guess you could argue that doing it in combine is better than peep2 and 
I'd agree with that.

> In fact, there are many more cases you could handle:
>
> You handle
>
> I1 -> I2 -> I3; I2 -> insn
>        I2 -> I3; I2 -> insn
>
> but there are also
>
>     I1,I2 -> I3; I2 -> insn
>
> and the many 4-insn combos, too.
Yes, but I wonder how much of this is really necessary in practice.  We 
could do exhaustive testing here, but I suspect the payoff isn't all 
that great.  Thus I'm comfortable with faulting in the cases we actually 
find are useful in practice.

> > +/* A is a compare (reg1, 0) and B is SINGLE_SET which SET_SRC is reg2.
> > +   It returns TRUE, if reg1 == reg2, and no other refer of reg1
> > +   except A and B.  */
>
> That sound like the only correct inputs are such a compare etc., but the
> routine tests whether that is true.
Correct, the RTL has to have a specific form and that is tested for. 
Comment updates can't hurt.


> > +static bool
> > +can_reuse_cc_set_p (rtx_insn *a, rtx_insn *b)
> > +{
> > +  rtx seta = single_set (a);
> > +  rtx setb = single_set (b);
> > +
> > +  if (BLOCK_FOR_INSN (a) != BLOCK_FOR_INSN (b)
>
> Neither the comment nor the function name mention this.  This test is
> better placed in the caller of this function, anyway.
Didn't consider it terribly important.  Moving it to the caller doesn't 
change anything significantly, though I would agree it's martinally cleaner.

> > @@ -3323,7 +3396,11 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> > *i1, rtx_insn *i0,
> >   	  rtx old = newpat;
> >   	  total_sets = 1 + extra_sets;
> >   	  newpat = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (total_sets));
> > -	  XVECEXP (newpat, 0, 0) = old;
> > +
> > +	  if (to_combined_insn)
> > +	    XVECEXP (newpat, 0, --total_sets) = old;
> > +	  else
> > +	    XVECEXP (newpat, 0, 0) = old;
> >   	}
>
> Is this correct?  If so, it needs a big fat comment, because it is
> not exactly obvious :-)
>
> Also, it doesn't handle at all the case where the new pattern already is
> a PARALLEL; can that never happen?
I'd convinced myself it was.  But yes, a comment here would be good.

Presumably you're thinking about a PARALLEL that satisfies single_set_p?

jeff