Fix PR 61225

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, December 10, 2014 3:16 AM
> To: Segher Boessenkool; Zhenqiang Chen
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Fix PR 61225
> 
> 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.

Updated. Check for a death note is enough since b is
prev_nonnote_nondebug_insn (a). 

> >
> >>>> +	  /* 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 :-)

Updated. 

> >>> 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.

Updated.

> >
> >> +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.

Updated.

> >
> >> @@ -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.

The following comments are added.

+	    /* This is a hack to match i386 instruction pattern, which
+		is like
+		    (parallel [
+			(set (reg:CCZ 17 flags)
+				...)
+			(set ...)})
+		we have to swap the newpat order of I3 and TO_COMBINED_INSN.
*/

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

No. It has nothing to do with single_set_p. I just want to reuse the code to
match the instruction pattern.

In common, the new PARALLEL is like

  Parallel
    newpat from I3
    newpat from I2 // if have
    newpat from I1 // if have
    newpat from I0 // if have

For to_combined_insn, i0 is NULL and there should have no

    newpat from I1

When handling I1->I2->I3, with normal order, it will get
  Parallel
    newpat from I3

After I2-> to_combined_insn, the parallel will be
  Parallel
    newpat from I3
    newpat from to_combined_insn.

But this can not match the insn pattern. So I swap the order to.
  Parallel
    newpat from to_combined_insn.
    newpat from I3

Here is the updated patch.

+/* { dg-final { cleanup-rtl-dump "combine" } } */

Fix PR 61225

Commit Message

Comments

Patch