[PATCH][MIPS][Loongson][SIMD] fix reduc_uplus_v8qi (biadd)

[Richard Sandiford]

Hi,

Sorry for the slow response.

Ruan Beihong <ruanbeihong@gmail.com> writes:
> 2009/2/4 Ruan Beihong <ruanbeihong@gmail.com>:
>> The standard name 'reduc_uplus_m' require the same mode of input and output.
>> When compiling some file in Mplayer, following error occurred:
>>
>> libfaad2/sbr_hfgen.c: In function 'hf_generation':
>> libfaad2/sbr_hfgen.c:184: error: unrecognizable insn:
>> (insn 562 561 563 69 libfaad2/sbr_hfgen.c:98 (set (reg:V8QI 618)
>>        (unspec:V4HI [
>>                (reg:V8QI 403 [ vect_var_.214 ])
>>            ] 515)) -1 (nil))
>> libfaad2/sbr_hfgen.c:184: internal compiler error: in extract_insn, at
>> recog.c:2027
>>
>> This patch fix the problem.
>>
>> The patch attached below:
>> Index: gcc/config/mips/loongson.md
>> ===================================================================
>> --- gcc/config/mips/loongson.md (revision 143919)
>> +++ gcc/config/mips/loongson.md (working copy)
>> @@ -358,10 +358,13 @@
>>   [(set_attr "type" "fadd")])
>>
>>  ;; Sum of unsigned byte integers.
>> +;; NOTE: reduce_uplus_m need the same mode of input and output,
>> +;; while biadd here actually return a V4HI with the lowerest field
>> +;; set to the sum of 8 byte.
>>  (define_insn "reduc_uplus_<mode>"
>> -  [(set (match_operand:<V_stretch_half> 0 "register_operand" "=f")
>> -        (unspec:<V_stretch_half> [(match_operand:VB 1 "register_operand" "f")]
>> -                                UNSPEC_LOONGSON_BIADD))]
>> +  [(set (match_operand:VB 0 "register_operand" "=f")
>> +        (unspec:VB [(match_operand:VB 1 "register_operand" "f")]
>> +                   UNSPEC_LOONGSON_BIADD))]
>>   "TARGET_HARD_FLOAT && TARGET_LOONGSON_VECTORS"
>>   "biadd\t%0,%1"
>>   [(set_attr "type" "fabs")])
>>
>> --
>> James Ruan
>>
>
>
> Ok. Here is the finished patch:
>
> Index: config/mips/loongson.md
> ===================================================================
> --- config/mips/loongson.md     (revision 143947)
> +++ config/mips/loongson.md     (working copy)
> @@ -358,7 +358,19 @@
>    [(set_attr "type" "fadd")])
>
>  ;; Sum of unsigned byte integers.
> -(define_insn "reduc_uplus_<mode>"
> +;; NOTE: reduce_uplus_m need the same mode of input and output,
> +;; while biadd here actually return a V4HI with the lowerest field
> +;; set to the sum of 8 byte.
> +(define_insn "reduc_uplus_v8qi"
> +  [(set (match_operand:VB 0 "register_operand" "=f")
> +        (unspec:VB [(match_operand:VB 1 "register_operand" "f")]
> +                  UNSPEC_LOONGSON_BIADD))]
> +  "TARGET_HARD_FLOAT && TARGET_LOONGSON_VECTORS"
> +  "biadd\t%0,%1"
> +  [(set_attr "type" "fabs")])
> +
> +;; This is the actual biadd which can be used through builtin function call.
> +(define_insn "loongson_biadd"
>    [(set (match_operand:<V_stretch_half> 0 "register_operand" "=f")
>          (unspec:<V_stretch_half> [(match_operand:VB 1 "register_operand" "f")]
>                                  UNSPEC_LOONGSON_BIADD))]

It occurred to me later that the MIPS backend doesn't allow an FPR
value to be treated as both a V4HI and a V8QI value.  That is, an FPR
set in one mode cannot be used in the other mode; we would first have
to move the FPR into a GPR and back again(!).  Using "biadd" for
reduc_uplus_v8qi seems to violate that.

So the question is: does the Loongson ISA specification allow vectors to
be reinterpreted in this way or not?  I imagine it does, in which case:

  (a) your patch is right and
  (b) mips_cannot_change_mode_class should return false when FROM and TO
      are integer vector modes of equal size.

But we should make certain that this is explicitly allowed first.
(This is a tricky area because the MIPS ISA _doesn't_ allow these kinds
of reinterpretation for the standard FPR formats (W, L, S, PS, etc.))

I'm a bit reluctant to have (a) without (b) because of the inconsistency.
I'm also a bit reluctant to do (b) at this stage in the 4.4 cycle because
it's a relatively invasive change.

I think the safest thing for 4.4 is to drop the reduc optimisation
altogether.  There'd then be plenty of time to improve things for 4.5.
(This includes the FPR logical ops you posted earlier.  It'd be great
to get those in 4.5 too.)

Here's the patch I'm testing and would like to apply for 4.4.
My testing is on non-Loongson, so is only a sanity check at best.

Does that sound OK?

Richard


gcc/
2009-02-xx  Ruan Beihong  <ruanbeihong@gmail.com>
	    Richard Sandiford  <rdsandiford@googlemail.com>

	* config/mips/mips.c (CODE_FOR_loongson_biadd): Delete.
	* config/mips/loongson.md (reduc_uplus_<mode>): Rename to...
	(loongson_biadd): ...this.

Index: gcc/config/mips/loongson.md
===================================================================
--- gcc/config/mips/loongson.md	2009-02-14 10:27:37.000000000 +0000
+++ gcc/config/mips/loongson.md	2009-02-14 10:27:45.000000000 +0000
@@ -358,7 +358,7 @@ (define_insn "loongson_pasubub"
   [(set_attr "type" "fadd")])
 
 ;; Sum of unsigned byte integers.
-(define_insn "reduc_uplus_<mode>"
+(define_insn "loongson_biadd"
   [(set (match_operand:<V_stretch_half> 0 "register_operand" "=f")
         (unspec:<V_stretch_half> [(match_operand:VB 1 "register_operand" "f")]
 				 UNSPEC_LOONGSON_BIADD))]
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2009-02-14 10:27:19.000000000 +0000
+++ gcc/config/mips/mips.c	2009-02-14 10:27:25.000000000 +0000
@@ -11253,7 +11253,6 @@ #define CODE_FOR_loongson_pminsh CODE_FO
 #define CODE_FOR_loongson_pminub CODE_FOR_uminv8qi3
 #define CODE_FOR_loongson_pmulhuh CODE_FOR_umulv4hi3_highpart
 #define CODE_FOR_loongson_pmulhh CODE_FOR_smulv4hi3_highpart
-#define CODE_FOR_loongson_biadd CODE_FOR_reduc_uplus_v8qi
 #define CODE_FOR_loongson_psubw CODE_FOR_subv2si3
 #define CODE_FOR_loongson_psubh CODE_FOR_subv4hi3
 #define CODE_FOR_loongson_psubb CODE_FOR_subv8qi3