Re: define_peepholes in mn10300

[Jeff Law <law at redhat dot com>]

 On 08/09/10 07:28, Steven Bosscher wrote:
> Hi Jeff,
>
> I'm looking at the remaining text peepholes (define_peephole instead
> of define_peephole2) and I have a few questions about mn10300, that
> you are a maintainer of.
I've been out on FMLA, so sorry for the late response...

> The first peephole is this:
>
> ;; Try to combine consecutive updates of the stack pointer (or any
> ;; other register for that matter).
> (define_peephole
>    [(set (match_operand:SI 0 "register_operand" "=dxay")
>          (plus:SI (match_dup 0)
>                   (match_operand 1 "const_int_operand" "")))
>     (set (match_dup 0)
>          (plus:SI (match_dup 0)
>                   (match_operand 2 "const_int_operand" "")))]
>    ""
>    "*
> {
>    operands[1] = GEN_INT (INTVAL (operands[2]) + INTVAL (operands[1]));
>    return \"add %1,%0\";
> }"
>    [(set_attr "cc" "clobber")])
>
> It seems to me that we have the CSA pass for this
> (combine-stack-adj.c). Maybe not for the "or any other register" part,
> but that should never happen anyway, or one of the CSE passes has not
> done its job properly. The peephole appears to pre-date the CSA pass.
> I would like to eliminate this define_peephole. Do you agree? If so,
> what would be sufficient testing for you to accept a patch that
> removes this peephole?
That peephole almost certainly pre-dates CSA; it's at least a decade or 
more old.  I have no idea if it ever triggered for other registers, 
supporting them was trivial and most likely an afterthought once I had 
the peephole eliminating useless SP adjustments.

One could even question if it was ever terribly important for SP 
adjustments given the mn103 is an ACCUMULATE_OUTGOING_ARGS machine.  
Though I'm certain I wouldn't have written the peephole without seeing 
code which would clearly benefit.

When the mn103 port was first developed, the primary concern was 
codesize and I most often used the runtime libraries (newlib/libstdc++) 
to tune as I didn't have any customer code to benchmark at the time.  So 
ISTM a build of newlib/libstdc++ with an abort placed in the C-code 
section of the peephole ought to be sufficient to determine it doesn't 
trigger.

> The second question is about the remaining define_peepholes, that all
> look more-or-less alike. Here is the first one:
>
> (define_peephole
>    [(set (cc0) (compare (match_operand:SI 0 "register_operand" "dx")
>                         (const_int 0)))
>     (set (pc) (if_then_else (ge (cc0) (const_int 0))
>                             (match_operand 1 "" "")
>                             (pc)))]
>    "dead_or_set_p (ins1, operands[0])&&  REG_OK_FOR_INDEX_P (operands[0])"
>    "add %0,%0\;bcc %1"
>    [(set_attr "cc" "clobber")])
>
>
> As far as I understand, the REG_OK_FOR_INDEX_P check is redundant:
>
> * The constraints "dx" require a data register or an extended register
>
> * REG_OK_FOR_INDEX_P(X) during peephoel is equivalent to
> REGNO_STRICT_OK_FOR_INDEX_P(X,REG_STRICT)
> * REGNO_STRICT_OK_FOR_INDEX_P requires REGNO_DATA_P or REGNO_EXTENDED_P.
>
> So the constrains should already make sure that REG_OK_FOR_INDEX_P is valid.
> Am I missing something?
I don't think you're missing anything.  Your logic appears sound.

> I would like to convert these remaining define_peepholes to
> define_peephole2s instead. However, I can't find a define_insn that
> produces the bcs or bcc instructions. Could use a little help figuring
> out what insn I should generate in the peephole2.
It's been a long time since I dealt with this aspect of porting, but 
isn't it the case that most ports don't expose branch-on-carry-set 
branch-on-carry-clear?   It looks like the mn103 was recently changed to 
not use cc0, which is definitely a good thing.  I'm not sure offhand the 
best way to recode this optimization in that new world.

Jeff