Re: head: MIPS: Complete the R4000 multiply/shift errata workaround

[Richard Sandiford <rsandifo at redhat dot com>]

"Maciej W. Rozycki" <macro@ds2.pg.gda.pl> writes:
> @@ -232,7 +232,8 @@
>  
>  	 ;; The r4000 multiplication patterns include an mflo instruction.
>  	 (and (eq_attr "type" "imul")
> -	      (ne (symbol_ref "TARGET_MIPS4000") (const_int 0)))
> +	      (and (ne (symbol_ref "TARGET_MIPS4000") (const_int 0))
> +		   (eq (symbol_ref "TARGET_MIPS16") (const_int 0))))
>  	 (const_string "hilo")
>  
>  	 (and (eq_attr "type" "hilo")

Notwithstanding cgd's comments about the target flag, I'd really like
to see this controlled by a single macro internally.  Something like
TARGET_FIX_R4000.  Better that than have the TARGET_MIPS16 check
duplicated everywhere.

> +;; The original R4000 has a cpu bug.  If a double-word or a variable
> +;; shift executes while a multiply is in progress, it may give an
> +;; incorrect result.  Avoid this by keeping the mflo with the mult on
> +;; the R4000.

Minor niggle, but maybe clarify that "it" refers to the shift?
I know it's obvious from what follows, but still.

> -(define_insn "muldi3_internal2"
> +(define_insn "muldi3_r4000"
>    [(set (match_operand:DI 0 "register_operand" "=d")
>  	(mult:DI (match_operand:DI 1 "register_operand" "d")
>  		 (match_operand:DI 2 "register_operand" "d")))
>     (clobber (match_scratch:DI 3 "=h"))
>     (clobber (match_scratch:DI 4 "=l"))]
> -  "TARGET_64BIT && (GENERATE_MULT3_DI || TARGET_MIPS4000)"
> -{
> -  if (GENERATE_MULT3_DI)
> -    return "dmult\t%0,%1,%2";
> -  else
> -    return "dmult\t%1,%2\;mflo\t%0";
> -}
> +  "TARGET_64BIT && (TARGET_MIPS4000 && !TARGET_MIPS16)"
> +  "dmult\t%1,%2\;mflo\t%0"
>    [(set_attr "type"	"imul")
>     (set_attr "mode"	"DI")
> -   (set (attr "length")
> -	(if_then_else (ne (symbol_ref "GENERATE_MULT3_DI") (const_int 0))
> -		      (const_int 4)
> -		      (const_int 8)))])
> +   (set_attr "length"	"8")])

Splitting the pattern into two looks like a cosmetic change.  Any particular
reason for it?  I kind-of prefer the way it is now.  The GENERATE_MULT3_DI
version is doing the same thing, the differences are just internal
implementation details.

The rest of my comments are about the *sidi3 and *highpart stuff.  The
first question is: is it really worth adding all this code for such an
ancient processor?  It would be much easier to disable everything expect
mulsi3 and muldi3 if TARGET_FIX_R4000.  Like you say:

> I give priority to correctness over performance and flexibility.

And who can argue with that? ;)

Assuming you really do want to keep these patterns:

> +(define_insn "mulsidi3_32bit_r4000"
> +  [(set (match_operand:DI 0 "register_operand" "=x")
> +	(mult:DI
> +	   (sign_extend:DI (match_operand:SI 1 "register_operand" "d"))
> +	   (sign_extend:DI (match_operand:SI 2 "register_operand" "d"))))]
> +  "!TARGET_64BIT && (TARGET_MIPS4000 && !TARGET_MIPS16)"
> +  "mult\t%1,%2\;mflo\t%."
> +  [(set_attr "type"	"imul")
> +   (set_attr "mode"	"SI")
> +   (set_attr "length"	"8")])
> +

I'd prefer to see this write directly into GPRs.  Something like:

    mult\t%1,%2\;mflo\t%L0\;mfhi\t%M0

should work.

Please find a way of factoring:

> +   ;; OP0 <- HI
> +   (set (match_dup 0) (match_dup 6))
> +
> +   ;; Zero-extend OP7.
> +   (set (match_dup 7)
> +	(ashift:DI (match_dup 7)
> +		   (const_int 32)))
> +   (set (match_dup 7)
> +	(lshiftrt:DI (match_dup 7)
> +		     (const_int 32)))
> +
> +   ;; Shift OP0 into place.
> +   (set (match_dup 0)
> +	(ashift:DI (match_dup 0)
> +		   (const_int 32)))
> +
> +   ;; OR the two halves together
> +   (set (match_dup 0)
> +	(ior:DI (match_dup 0)
> +		(match_dup 7)))]
> +  ""
> +  [(set_attr "type"	"imul")
> +   (set_attr "mode"	"SI")
> +   (set_attr "length"	"24")])

since most of it is just cut&paste.  One idea is to use your new
version for all targets and turn:

> +(define_insn "*mulsidi3_64bit_parts_r4000"
> +  [(set (match_operand:DI 0 "register_operand" "=d")
> +	(sign_extend:DI
> +	   (mult:SI (match_operand:SI 2 "register_operand" "d")
> +		    (match_operand:SI 3 "register_operand" "d"))))
> +   (set (match_operand:DI 1 "register_operand" "=h")
> +	(ashiftrt:DI
> +	   (mult:DI
> +	      (match_operator:DI 4 "extend_operator" [(match_dup 2)])
> +	      (match_operator:DI 5 "extend_operator" [(match_dup 3)]))
> +	   (const_int 32)))
> +   (clobber (match_operand:DI 6 "register_operand" "=l"))]
> +  "TARGET_64BIT && (TARGET_MIPS4000 && !TARGET_MIPS16)
> +   && GET_CODE (operands[4]) == GET_CODE (operands[5])"
> +{
> +  if (GET_CODE (operands[4]) == SIGN_EXTEND)
> +    return "mult\t%2,%3\;mflo\t%0";
> +  else
> +    return "multu\t%2,%3\;mflo\t%0";
> +}
> +  [(set_attr "type"	"imul")
> +   (set_attr "mode"	"SI")
> +   (set_attr "length"	"8")])
> +

into a define_inan_nand_split whose split condition is "&& !TARGET_FIX_R4000".
Not sure without testing whether this would work.

> +(define_insn "umulsi3_highpart_r4000"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +	(truncate:SI
> +	 (lshiftrt:DI
> +	  (mult:DI (zero_extend:DI (match_operand:SI 1 "register_operand" "d"))
> +		   (zero_extend:DI (match_operand:SI 2 "register_operand" "d")))
> +	  (const_int 32))))
> +   (clobber (match_scratch:SI 3 "=l"))
> +   (clobber (match_scratch:SI 4 "=h"))]
> +  "!ISA_HAS_MULHI && (TARGET_MIPS4000 && !TARGET_MIPS16)"
> +  "multu\t%1,%2\;mflo\t%0"
> +  [(set_attr "type"	"imul")
> +   (set_attr "mode"	"SI")
> +   (set_attr "length"	"8")])
> +

Shouldn't this be mfhi?

> +(define_insn "smulsi3_highpart_r4000"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +	(truncate:SI
> +	 (lshiftrt:DI
> +	  (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand" "d"))
> +		   (sign_extend:DI (match_operand:SI 2 "register_operand" "d")))
> +	  (const_int 32))))
> +   (clobber (match_scratch:SI 3 "=l"))
> +   (clobber (match_scratch:SI 4 "=h"))]
> +  "!ISA_HAS_MULHI && (TARGET_MIPS4000 && !TARGET_MIPS16)"
> +  "mult\t%1,%2\;mflo\t%0"
> +  [(set_attr "type"	"imul")
> +   (set_attr "mode"	"SI")
> +   (set_attr "length"   "8")])
> +

Likewise.  If we're going to have these patterns, I'd prefer to
reintroduce extend_operator and use common patterns for smul* and umul*.
Not a requirement for your patch though.

Richard