Re: Resubmit/ping: peephole2 vs cond-exec vs df

[Richard Henderson <rth@redhat.com>]

On 06/28/2010 05:26 PM, Bernd Schmidt wrote:
> On 06/29/2010 01:58 AM, Richard Henderson wrote:
>>> +  /* If an insn has RTX_FRAME_RELATED_P set, peephole substitution would lose
>>> +     the REG_FRAME_RELATED_EXPR that is attached.  */
>>> +  if (RTX_FRAME_RELATED_P (insn))
>>> +    {
>>> +      /* Let the buffer drain first.  */
>>> +      if (peep2_current_count > 0)
>>> +	return false;
>>> +      df_simulate_one_insn_forwards (bb, insn, live);
>>> +      return true;
>>
>> You've changed the logic so that frame-related insns are allowed to be
>> peepholed, so long as they begin a match.  Previously we skipped these
>> insns entirely, which I think is more correct.
> 
> I'm not sure this is true.  It never adds a RTX_FRAME_RELATED_P insn to
> the buffer, it just returns true so that we process the next insn?
> 
> I admit the buffer logic is a bit convoluted, and I'd welcome
> suggestions how to make it clearer.  An explicit state machine?

Ah, I missed that we didn't add the insn to the buffer.  I think
all that would be needed is a comment here to that effect.

>> Why do you wait until you've completely filled the buffer before trying
>> a match?  Doesn't this lead to useless work?  Or does this on average
>> pay off because of less calls into peep2_recog (although more calls into
>> df scanning insns)?
> 
> To ensure that we always match the longest possible peephole if there
> are similar ones.

A worthy goal; I hadn't thought of that.  Another comment to add?

> length, but that won't always work if we start to match against
> partially filled buffers.  It's 2am so I may be blind but I don't see
> how it causes more calls into df_simulate or other useless work?  There
> should be exactly one such call per insn as it is added to the buffer
> (plus any updates necessary after we match something and have to
> regenerate life information).  Life information is stored in the buffer
> so we don't have to do anything when advancing the buffer start.

The case in which we have matches is what I had in mind.  Say the
buffer has length 10.  If we fill the buffer then match the first
insn, that invalidates the life information for the entire buffer
does it not?  In which case we've wasted effort on 9 insns.

Unless we deem the peep2 life changes to be strictly local to the
region matched, in which case we can simply re-use the life info
computed the first time around.  I haven't gone back to the patch 
to see if that's what's going on...


r~