Re: [PATCH, MIPS] Merge seb/seh into regular sign-extend pattern

[Adam Nemet <anemet at caviumnetworks dot com>]

Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
> > I am working on a patch to generate dext for zero-extension on MIPS64r2.  This
> > is very similar generating seb/seh for sign-extension.  While implementing
> > that I was surprised to see that seb/seh weren't generated from the regular
> > sign-extend define_insn_and_split.  This patch does that.
> 
> Hmm.  If this is purely a stylistic change, then TBH the current version
> seems a little clearer and more consistent.  The new version has to
> check in three places whether we actually have an SEB/SEH instruction,
> and it still leaves the GENERATE_MIPS16E case for a separate pattern,
> even though it's very similar in style to the non-MIPS16 SEB/SEH case.

Yes, it's stylistic only.

Why do you say that I need to check SEB/SEH in three places.  I check it in
two places just like any other define_insn_and_split that only splits
conditionally: whether to split as RTL and when emitting assembly.

The new form of the pattern does not seem any different from how I did
ISA_HAS_EXTS support in other define_insn_and_splits
(e.g. *extenddi_truncate<mode>").  The only difference it that actual insn is
figured out in the attribute section and not in the output template part of
the pattern.  (I actually consider that an improvement -- but I like lisp
dialects ;).)  Do you have issue with the additional attr->insn mapping here?

> I'm happy to consider it if it is needed by your later patch.

No it's not.

> It just doesn't seem like an improvement on its own.

It's always better to have things in one pattern that can go into one pattern
(for example I think that a !GENERATE_MIPS16E is currently missing on the se
pattern for consistency) and I think this nicely summarizes when we emit a
load, seb/seh or the double shift.  But maybe this is just bias on my part
because I wrote it ;).

> If you do want to combine the patterns, another alternative might be to
> use the "enabled" attribute, which wasn't available when these patterns
> (or most of the others in mips.md) were written.  E.g.:
> 
> ;; should go at the end of the attribute list, before the asm attributes.
> (define_attr "enabled" ""
>   (cond [(eq_attr "type" "signext")
>     	 (symbol_ref "ISA_HAS_SEB_SEH")]
>         (const_int 1)))
> 
> (define_insn_and_split "*extend<SHORT:mode><GPR:mode>2"
>   [(set (match_operand:GPR 0 "register_operand" "=d,d,d")
>         (sign_extend:GPR
> 	     (match_operand:SHORT 1 "nonimmediate_operand" "d,d,m")))]
>   "!GENERATE_MIPS16E"
>   "@
>    se<SHORT:size>\t%0,%1
>    #
>    l<SHORT:size>\t%0,%1"
>   "&& reload_completed && REG_P (operands[1]) && !ISA_HAS_SEB_SEH""
>   [(set (match_dup 0) (ashift:GPR (match_dup 1) (match_dup 2)))
>    (set (match_dup 0) (ashiftrt:GPR (match_dup 0) (match_dup 2)))]
> {
>   operands[1] = gen_lowpart (<GPR:MODE>mode, operands[1]);
>   operands[2] = GEN_INT (GET_MODE_BITSIZE (<GPR:MODE>mode)
> 			 - GET_MODE_BITSIZE (<SHORT:MODE>mode));
> }
>   [(set_attr "move_type" "signext,shift_shift,load")
>    (set_attr "mode" "<GPR:MODE>")])
> 
> which at least cuts the checks down from 3N to N+1.  With a bit of work
> we could include the MIPS16e version too.  But whether we include
> MIPS16 or not, I suppose the drawback of this approach is that there's
> even more magic going on under the hood.

Also if we have complexitiy I rather have it in expressive lisp-like attribute
expressions than in constraints unless of course it's required for constraints
in the first place (which is true for mips16 but not in this case).

Adam