Patchwork [vect] Ask for review and approving the patch about vect and loongson

login
register
mail settings
Submitter Eric Fisher
Date July 12, 2010, 9:49 a.m.
Message ID <AANLkTikl_l9soiXnXjVJU081kFyWu02k8M2ix0F1unUY@mail.gmail.com>
Download mbox | patch
Permalink /patch/58588/
State New
Headers show

Comments

Eric Fisher - July 12, 2010, 9:49 a.m.
2010/7/10 Richard Sandiford <rdsandiford@googlemail.com>:
> The updated vect.patch is OK.
>
> The target-supports.patch is OK if you break the line immediately before
> the "&&"; the lines are too long as-is.

OK, I've updated the patch in the attachment.

> As far as loongson.patch goes:
>
> Index: config/mips/loongson.md
> ===================================================================
> --- config/mips/loongson.md     (revision 161865)
> +++ config/mips/loongson.md     (working copy)
> @@ -352,6 +352,16 @@
>   "pmulh<V_suffix>\t%0,%1,%2"
>   [(set_attr "type" "fmul")])
>
> +;; Standard pattern mulm3
> +(define_expand "mul<mode>3"
> +  [(set (match_operand:VH 0 "register_operand" "=f")
> +        (unspec:VH [(match_operand:VH 1 "register_operand" "f")
> +                    (match_operand:VH 2 "register_operand" "f")]
> +                   UNSPEC_LOONGSON_PMULL))]
> +  "TARGET_HARD_FLOAT && TARGET_LOONGSON_VECTORS"
> +  "")
> +
> +
>  ;; Multiply signed integers and store low result.
>  (define_insn "loongson_pmull<V_suffix>"
>   [(set (match_operand:VH 0 "register_operand" "=f")
>
> Instead rename loongson_pmull<V_suffix> to mul<mode>3 and add #defines
> to mips.c to make CODE_FOR_loongson_pmullh an alias for CODE_FOR_mulhi3,
> etc.  Grep for CODE_FOR_loongson to see where I mean.

Thanks for the helpful information. I've changed the patch in the attachment.

> The patch adds patterns for shifts right but not shifts left.  Was that
> deliberate?

Oh, I missed it. Now I've added it.

> The same rename-and-#define common would apply to the shifts, but I'm
> worried.  The loongson insns are all "infinite precision" shifts
> (any V2HI << 16 == 0), whereas MIPS is a SHIFT_COUNT_TRUNCATED
> target (implying V2HI << 16 should be an identity operation).
> This could in theory cause us to miscompile things like:
>
>   v2hi >> (shift & 15)
>
> and although I can't come up with a testcase, I think the problem
> is still there.
>
> Admittedly this means the current code is wrong too.  It should be
> using UNSPECs instead of shift rtxes.
>
> One fix would be to make SHIFT_COUNT_TRUNCATED take a mode argument
> (and turn it into a target hook at the same time).
>
> Richard
>

I'm going to take a look at this problem. Do you think that it is all
right to first submit the current patch and leave this problem in the
next one separately?

Thanks
Eric
Richard Sandiford - July 12, 2010, 7:21 p.m.
Eric Fisher <joefoxreal@gmail.com> writes:
> 2010/7/10 Richard Sandiford <rdsandiford@googlemail.com>:
>> The updated vect.patch is OK.
>>
>> The target-supports.patch is OK if you break the line immediately before
>> the "&&"; the lines are too long as-is.
>
> OK, I've updated the patch in the attachment.

Looks good.  OK to apply.

>> As far as loongson.patch goes:
>>
>> Index: config/mips/loongson.md
>> ===================================================================
>> --- config/mips/loongson.md     (revision 161865)
>> +++ config/mips/loongson.md     (working copy)
>> @@ -352,6 +352,16 @@
>>   "pmulh<V_suffix>\t%0,%1,%2"
>>   [(set_attr "type" "fmul")])
>>
>> +;; Standard pattern mulm3
>> +(define_expand "mul<mode>3"
>> +  [(set (match_operand:VH 0 "register_operand" "=f")
>> +        (unspec:VH [(match_operand:VH 1 "register_operand" "f")
>> +                    (match_operand:VH 2 "register_operand" "f")]
>> +                   UNSPEC_LOONGSON_PMULL))]
>> +  "TARGET_HARD_FLOAT && TARGET_LOONGSON_VECTORS"
>> +  "")
>> +
>> +
>>  ;; Multiply signed integers and store low result.
>>  (define_insn "loongson_pmull<V_suffix>"
>>   [(set (match_operand:VH 0 "register_operand" "=f")
>>
>> Instead rename loongson_pmull<V_suffix> to mul<mode>3 and add #defines
>> to mips.c to make CODE_FOR_loongson_pmullh an alias for CODE_FOR_mulhi3,
>> etc.  Grep for CODE_FOR_loongson to see where I mean.
>
> Thanks for the helpful information. I've changed the patch in the attachment.

Sorry, I forgot to ask you to change the pattern from an UNSPEC to
a MULT at the same time.  The multiplication part is OK with that fixed.

>> The same rename-and-#define common would apply to the shifts, but I'm
>> worried.  The loongson insns are all "infinite precision" shifts
>> (any V2HI << 16 == 0), whereas MIPS is a SHIFT_COUNT_TRUNCATED
>> target (implying V2HI << 16 should be an identity operation).
>> This could in theory cause us to miscompile things like:
>>
>>   v2hi >> (shift & 15)
>>
>> and although I can't come up with a testcase, I think the problem
>> is still there.
>>
>> Admittedly this means the current code is wrong too.  It should be
>> using UNSPECs instead of shift rtxes.
>>
>> One fix would be to make SHIFT_COUNT_TRUNCATED take a mode argument
>> (and turn it into a target hook at the same time).
>>
>> Richard
>
> I'm going to take a look at this problem. Do you think that it is all
> right to first submit the current patch and leave this problem in the
> next one separately?

No, I'm afraid the truncation needs to be sorted out before the shift
changes go in.  As things stand, the shift changes would in principle
introduce a wrong-code regression.

Richard

Patch

Index: config/mips/loongson.md
===================================================================
--- config/mips/loongson.md	(revision 162065)
+++ config/mips/loongson.md	(working copy)
@@ -353,7 +353,7 @@ 
   [(set_attr "type" "fmul")])
 
 ;; Multiply signed integers and store low result.
-(define_insn "loongson_pmull<V_suffix>"
+(define_insn "mul<mode>3"
   [(set (match_operand:VH 0 "register_operand" "=f")
         (unspec:VH [(match_operand:VH 1 "register_operand" "f")
 		    (match_operand:VH 2 "register_operand" "f")]
@@ -413,7 +413,7 @@ 
   [(set_attr "type" "fmul")])
 
 ;; Shift left logical.
-(define_insn "loongson_psll<V_suffix>"
+(define_insn "ashl<mode>3"
   [(set (match_operand:VWH 0 "register_operand" "=f")
         (ashift:VWH (match_operand:VWH 1 "register_operand" "f")
 		    (match_operand:SI 2 "register_operand" "f")))]
@@ -422,7 +422,7 @@ 
   [(set_attr "type" "fmul")])
 
 ;; Shift right arithmetic.
-(define_insn "loongson_psra<V_suffix>"
+(define_insn "ashr<mode>3"
   [(set (match_operand:VWH 0 "register_operand" "=f")
         (ashiftrt:VWH (match_operand:VWH 1 "register_operand" "f")
 		      (match_operand:SI 2 "register_operand" "f")))]
@@ -431,7 +431,7 @@ 
   [(set_attr "type" "fdiv")])
 
 ;; Shift right logical.
-(define_insn "loongson_psrl<V_suffix>"
+(define_insn "lshr<mode>3"
   [(set (match_operand:VWH 0 "register_operand" "=f")
         (lshiftrt:VWH (match_operand:VWH 1 "register_operand" "f")
 		      (match_operand:SI 2 "register_operand" "f")))]
Index: config/mips/mips.c
===================================================================
--- config/mips/mips.c	(revision 162065)
+++ config/mips/mips.c	(working copy)
@@ -12667,6 +12667,13 @@  AVAIL_NON_MIPS16 (cache, TARGET_CACHE_BU
 #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_pmullh CODE_FOR_mulv4hi3
+#define CODE_FOR_loongson_psllh CODE_FOR_ashlv4hi3
+#define CODE_FOR_loongson_psllw CODE_FOR_ashlv2si3
+#define CODE_FOR_loongson_psrlh CODE_FOR_lshrv4hi3
+#define CODE_FOR_loongson_psrlw CODE_FOR_lshrv2si3
+#define CODE_FOR_loongson_psrah CODE_FOR_ashrv4hi3
+#define CODE_FOR_loongson_psraw CODE_FOR_ashrv2si3
 #define CODE_FOR_loongson_psubw CODE_FOR_subv2si3
 #define CODE_FOR_loongson_psubh CODE_FOR_subv4hi3
 #define CODE_FOR_loongson_psubb CODE_FOR_subv8qi3