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

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

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