This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [MIPS] [Loongson] Specific integer instructions and prefetch added


Thanks for the update.

"Ruan Beihong" <ruanbeihong@gmail.com> writes:
> 2008/11/11 Richard Sandiford <rdsandiford@googlemail.com>:
>> Formatting; this should be:
>>
>>    {
>>      if (TARGET_LOONGSON_2EF)
>>        emit_insn (gen_mul<mode>3_mul3_ls2ef (operands[0], operands[1],
>>                                              operands[2]));
>>      else
>>        emit_insn (gen_mul<mode>3_mul3 (operands[0], operands[1], operands[2]));
>>    }
>>
>> (GCC's quite strict about formatting.)  But I don't see any reason to make
>> the TARGET_LOONSON_2EF code depend on ISA_HAS_<D>MUL3; it seems easier
>> to have:
>>
>>  if (TARGET_LOONGSON_2EF)
>>    emit_insn (gen_mul<mode>3_mul3_ls2ef (operands[0], operands[1],
>>                                          operands[2]));
>>  else if (ISA_HAS_<D>MUL3)
>>    emit_insn (gen_mul<mode>3_mul3 (operands[0], operands[1], operands[2]));
>>  else if (TARGET_FIX_R4000)
>>    ...

In the patch you had:

  if(TARGET_LOONGSON_2EF)
    emit_insn(gen_mul<mode>3_mul3_ls2ef(operands[0], operands[1], operands[2]));

instead, but the coding standards require spaces before the "("s.
(See what I meant when I said GCC was quite strict about formatting? ;))
The code really should be formatted the way I had it above.

(Lines shouldn't be longer than 80 characters, which is why I split
the call over two lines.)

I know this must seem overly picky, but bear in mind that, if a hundred
people made changes in their own preferred style, the whole thing would
become a right mess.

>>> +  if(TARGET_LOONGSON_2EF)
>>> +  /*Loongson 2[ef] use load to $0 to perform prefetching*/
>>> +    return "ld\t$0,%a0";
>>
>> Formatting; should be:
>>
>>  if (TARGET_LOONGSON_2EF)
>>    /* Loongson 2[ef] use load to $0 to perform prefetching.  */
>>    return "ld\t$0,%a0";

Looks like you forgot this bit.  (Comments should start with a
captial letter and end with ".  ".)

>>> +(define_peephole2
>>> +  [(set (match_operand:SI 0 "lo_operand")
>>> +        (mult:SI (match_operand:SI 1 "d_operand")
>>> +                 (match_operand:SI 2 "d_operand")))
>>> +   (set (match_operand:SI 3 "d_operand")
>>> +        (match_dup 0))]
>>> +   "TARGET_LOONGSON_2EF && peep2_reg_dead_p(2, operands[0])"
>>> +  [(set (match_dup 3)
>>> +        (mult:SI (match_dup 1)
>>> +                 (match_dup 2)))]
>>
>> When do we need this?  How do we get a LO-target multiplication
>> after the mul<mode>3 change above?
>
> In traditional mips ISA, mult's result is stored in LO/HI.
> If we need to get the result, we need an extra mflo.
> Loongson2's multu.g is used to save the extra mflo in such case.
> So I want use this peephole2 to turn insns like:
>  mult $x, $y
>  mflo $z
> into
>  multu.g $z, $x, $y
> when the LO is used only once in such case.

Sure, but what I meant was: define_peephole2s operate on instructions
that have already been generated, and that already match some define_insn.
So this define_peephole2 is only necessary if we can still sometimes
generate a "normal" LO-based multiplication INSN for Loongson.
So how could that happen after your changes to the mul<mode>3 expanders?
Surely we should always be using mul<mode>3_mul3_ls2ef for Loongson,
in which case every multiplication will already write to a GPR
instead of LO.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]