RFA: MN10300: Replace cc0 with CC_REG

[Richard Henderson]

On 10/11/2010 01:55 PM, Nick Clifton wrote:
>   Attached is a patch to remove the use of the cc0 register from the
>   MN10300 port and replace it with a CC_REG hard register instead.  The
>   main reason for this patch is to allow better scheduling of MN10300
>   instructions, although it allows a tidy up of some of the code.  I
>   intend to follow up this patch, if accepted, with another that adds
>   scheduling information to the MN10300 backend.

There are unrelated changes in here.  It would have been easier if
you'd have done some of the other cleanup in separate patches.

That said...

>   { 0x03fc0000, 0 },	/* FP_ACC_REGS */	\
> + { 0x00000,    0x80000 },/* CC_REGS */		\
>   { 0x3fdff, 0 }, 	/* GENERAL_REGS */	\

Not a bug, but it would be helpful if these lined up ala %#08x.

> -		 (match_test "XEXP (op, 0) != stack_pointer_rtx"))
> +		 (match_test "REGNO (XEXP (op, 0)) != STACK_POINTER_REGNUM"))

Why?  The correlation is guaranteed.

>  (define_constants [
>    (PIC_REG	6)
>    (SP_REG	9)
> +  (CC_REG       51)

If you use define_c_enum you can avoid duplicating these numbers in cpu.h.

> +(define_expand "mulsidi3"
> +  [(parallel [(set (match_operand:DI                          0 "register_operand")
> +		   (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand"))
> +			    (sign_extend:DI (match_operand:SI 2 "register_operand"))))
> +	      (clobber (reg:CC CC_REG))
> +	     ])
> +  ]
> +  "TARGET_AM33"
> +  ""
> +)
> +
> +(define_insn "*mulsidi3_insn"
>    [(set (match_operand:DI 0 "register_operand" "=dax")
>          (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand" "dax"))
> -                 (sign_extend:DI (match_operand:SI 2 "register_operand" "dax"))))]
> +                 (sign_extend:DI (match_operand:SI 2 "register_operand" "dax"))))
> +   (clobber (reg:CC CC_REG))
> +  ]
>    "TARGET_AM33"
>    "mul %1,%2,%H0,%L0"
> -  [(set_attr "cc" "set_zn")])
> +)

Is there a reason to have the separate expander?  The same comment
appears to apply to several other of the basic named patterns.  As
distinguished from those that have both AM33 and non-am33 implementations.

> +(define_insn_and_split "*cbranchsi4_<code>"
> +  [(set (pc)
> +	(if_then_else (most_cond (match_operand:SI     0 "register_operand"  "dax")

Any reason to use macros here instead of match_operator?  It seems a
bit wasteful to generate 8 copies of these patterns...

> +;; We expand the comparison into a single insn so that it will not be split
> +;; up by reload.
>  (define_expand "cbranchsi4"

Do you not have an add that doesn't clobber flags?  Are you planning
to eliminate redundant compares via peepholes?

>  (define_insn_and_split "mn10300_loadPC"
>    [(parallel
>      [(set (reg:SI PIC_REG) (pc))
>       (use (match_operand 0 "" ""))])]
> -  ""
> +  "! TARGET_AM33"
>    "#"
>    "reload_completed"
>    [(match_operand 0 "" "")]
> -  "
>  {
> +    if (TARGET_AM33)
> +      emit_insn (gen_am33_loadPC (operands[0]));
> +    else
> +      {

Err, I think this should be "&& reload_completed".
That should cleanup the confusing TARGET_AM33 test
inside a pattern that's !TARGET_AM33.


r~