Re: [patch] Update MIPS DSP madd, maddu, msub, msubu, mult, multu

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Fu, Chao-Ying" <fu@mips.com> writes:
>> > Looks good.  My main concern is that the patch as-is introduces
>> > a dependency on bleeding-edge binutils.  That's not such a problem
>> > for cases where the user has explicitly called a
>> > __builtin_*() function
>> > in rev1 code, because that simply didn't work before.  But it
>> > is a problem
>> > for cases where the compiler is automatically using these
>> > instructions.
>> > You'll get assembler errors when compiling ordinary C code,
>> > which never
>> > looks good.
>> >
>> > I think we need to add a configure-time check to see whether the
>> > assembler has your patch.  There are quite a few existing examples
>> > of this.  Then we should introduce a new macro such as:
>> >
>> >     ISA_HAS_DSP_MULT
>> >
>> > This macro can be defined to ISA_HAS_DSP if the assembler
>> has your fix
>> > and ISA_HAS_DSPR2 otherwise.
>> >
>>
>>   I will update my patch to have a configure-time check and
>> ues ISA_HAS_DSP_MULT.
>> Thanks a lot!
>>
>   As you suggest, here is the revised patch.  These 6 instructions are moved to DSPr1, so that
> the corresponding builtin-funcitons can be called when "-mdsp" is used.
> The destination register may be hilo or four DSP accumulators, depending on
> the register constraint "ka" that depends on ISA_HAS_DSP_MULT.
> And ISA_HAS_DSP_MULT is based on HAVE_AS_DSP_REV1_MULT from configuration-time.
> I restore the test of dspr2-MULT.c and dspr2-MULTU.c to use -mdspr2, since -mdspr2 definitely
> can trigger the usage of all four accumulators.

The problem with this is that the condition on the expander:

+(define_expand "mips_mult<u>"
+  [(set (match_operand:DI 0 "register_operand")
+       (mult:DI
+        (any_extend:DI (match_operand:SI 1 "register_operand"))
+        (any_extend:DI (match_operand:SI 2 "register_operand"))))]
+  "ISA_HAS_DSP && !TARGET_64BIT")

doesn't match the condition on the associated insn:

 (define_insn "<u>mulsidi3_32bit"
-  [(set (match_operand:DI 0 "register_operand" "=x")
+  [(set (match_operand:DI 0 "register_operand" "=ka")
        (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
                 (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))]
-  "!TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DSPR2"
-  "mult<u>\t%1,%2"
+  "!TARGET_64BIT && !TARGET_FIX_R4000"
...

Note the !TARGET_FIX_R4000.  It might not make much sense, but there's
nothing stopping someone from using -mdsp and -mfix-r4000 together.
Although we _could_ either reject the combination or force MASK_FIX_R4000
to false, that would set a bad precendent.  In general, -mfix-* flags
should work "on top of" whatever ISA mode is selected.

However, I like your idea of making the builtin functions available in rev1
regardless of whether the assembler supports them, and falling back on the
single accumulator if necessary.  It seems reasonable to use:

  "!TARGET_64BIT && (!TARGET_FIX_R4000 || ISA_HAS_DSP)"

here, with a comment:

;; As well as being named patterns, these instructions are used by the
;; __builtin_mips_mult<u>() functions.  We must always make those functions
;; available if !TARGET_64BIT && ISA_HAS_DSP.

Also, now that you've merged the two mult define_insns (a good step,
thanks), there's no need for the mips_mult<u> expander.  Just add:

#define CODE_FOR_mips_mult CODE_FOR_mulsidi3_32bit
#define CODE_FOR_mips_multu CODE_FOR_umulsidi3_32bit

to the builtins code in mips.c.  Similarly, <u>maddsidi4 and
<u>msubsidi4 are now named patterns (which wasn't true when
the DSP code was originally added), so we can do the same
thing for mips_madd<u> and mips_msub<u>.

A similar problem (though not in this case a correctness problem)
occurs with:

-  "!TARGET_64BIT && (ISA_HAS_MSAC || GENERATE_MADD_MSUB || ISA_HAS_DSPR2)"
+  "!TARGET_64BIT && (ISA_HAS_MSAC || GENERATE_MADD_MSUB || ISA_HAS_DSP)"

GENERATE_MADD_MSUB is defined as:

#define GENERATE_MADD_MSUB      (ISA_HAS_MADD_MSUB && !TUNE_74K)

yet the 74k is a DSP target.  The old condition could be justified
on the basis that, if you're tuning for the 74k but have all four
accumulators available, it's better to use these instructions anyway.
After the patch, we might use the instructions when tuning for the
74k and when only one accumulator is available, which is exactly
the case that GENERATE_MADD_MSUB was trying to avoid.

Still, I suppose -mtune=74k -mdsp, as opposed to -mtune=74k -mdspr2,
is probably a niche combination, and I can't think of a good way out.
I suppose we can just hope that anyone using that combination is
prepared to upgrade their binutils or live the possible drop in
performance.

So, let's go with the MADD_MSUB conditions in your patch, but add:

;; As well as being named patterns, these instructions are used by the
;; __builtin_mips_madd<u>() functions.  We must always make those functions
;; available if !TARGET_64BIT && ISA_HAS_DSP.
;;
;; This leads to a slight inconsistency.  We honor any tuning overrides
;; in GENERATE_MADD_MSUB for -mno-dsp, but always ignore them for -mdsp,
;; even if !ISA_HAS_DSP_MULT.

before <u>maddsidi4, and:

;; See the comment above <u>maddsidi4 for the relationship between
;; ISA_HAS_DSP and ISA_HAS_DSP_MULT.

above <u>msubsidi4.

Also, a minor nit, but let's call the macro HAVE_AS_DSPR1_MULT,
for consistency with -mdspr2, ISA_HAS_DSPR2, etc.

Richard