Re: [PATCH] Fix PR 61225

[Jeff Law <law at redhat dot com>]

On 12/04/14 01:43, Zhenqiang Chen wrote:
> > > > >
> > > > >          Part of PR rtl-optimization/61225
> > > > >          * combine.c (refer_same_reg_p): New function.
> > > > >          (combine_instructions): Handle I1 -> I2 -> I3; I2 -> insn.
> > > > >          (try_combine): Add one more parameter TO_COMBINED_INSN, which
> > >is
> > > > >          used to create a new insn parallel (TO_COMBINED_INSN, I3).
> > > > >
> > > > >testsuite/ChangeLog:
> > > > >2014-08-04  Zhenqiang Chen<zhenqiang.chen@linaro.org>
> > > > >
> > > > >          * gcc.target/i386/pr61225.c: New test.
THanks for the updates and clarifications.  Just a few minor things and 
while it's a bit of a hack, I'll approve:



> +
> +/* 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.  */
> +
> +static bool
> +refer_same_reg_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)
> +      || !seta
> +      || !setb)
> +    return false;
> +
> +  if (GET_CODE (SET_SRC (seta)) != COMPARE
> +      || GET_MODE_CLASS (GET_MODE (SET_DEST (seta))) != MODE_CC
> +      || !REG_P (XEXP (SET_SRC (seta), 0))
> +      || XEXP (SET_SRC (seta), 1) != const0_rtx
> +      || !REG_P (SET_SRC (setb))
> +      || REGNO (SET_SRC (setb)) != REGNO (XEXP (SET_SRC (seta), 0)))
> +    return false;
Do you need to verify SETA and SETB satisfy single_set?  Or has that 
already been done elsewhere?

The name refer_same_reg_p seems wrong -- your function is verifying the 
underlying RTL store as well as the existence of a a dependency between 
the insns.  Can you try to come up with a better name?

Please use CONST0_RTX (mode)  IIRC that'll allow this to work regardless 
of the size of the modes relative to the host word size.



> +
> +  if (DF_REG_USE_COUNT (REGNO (SET_SRC (setb))) > 2)
> +    {
> +      df_ref use;
> +      rtx insn;
> +      unsigned int i = REGNO (SET_SRC (setb));
> +
> +      for (use = DF_REG_USE_CHAIN (i); use; use = DF_REF_NEXT_REG (use))
> +        {
> +	  insn = DF_REF_INSN (use);
> +	  if (insn != a && insn != b && !(NOTE_P (insn) || DEBUG_INSN_P (insn)))
> +	    return false;
> +	}
> +    }
> +
> +  return true;
> +}
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?

>   		  }
>   	      }
>
> +	  /* 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.  */
> +	  if ((prev = prev_nonnote_nondebug_insn (insn)) != NULL_RTX
> +	      && refer_same_reg_p (insn, prev)
> +	      && max_combine >= 4)
> +	    {
> +		struct insn_link *next1;
> +		FOR_EACH_LOG_LINK (next1, prev)
> +		  {
> +		    rtx_insn *link1 = next1->insn;
> +		    if (NOTE_P (link1))
> +		      continue;
> +		    /* I1 -> I2 -> I3; I2 -> insn;
> +		       output parallel (insn, I3).  */
> +		    FOR_EACH_LOG_LINK (nextlinks, link1)
> +		      if ((next = try_combine (prev, link1,
> +					       nextlinks->insn, NULL,
> +					       &new_direct_jump_p,
> +					       last_combined_insn, insn)) != 0)
> +
> +			{
> +			  delete_insn (insn);
> +			  insn = next;
> +			  statistics_counter_event (cfun, "four-insn combine", 1);
> +			  goto retry;
> +			}
> +		    /* I2 -> I3; I2 -> insn
> +		       output next = parallel (insn, I3).  */
> +		    if ((next = try_combine (prev, link1,
> +					     NULL, NULL,
> +					     &new_direct_jump_p,
> +					     last_combined_insn, insn)) != 0)
> +
> +		      {
> +			delete_insn (insn);
> +			insn = next;
> +			statistics_counter_event (cfun, "three-insn combine", 1);
> +			goto retry;
> +		      }
> +		  }
> +	    }
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)

Please make those changes and repost for final approval.

jeff