This is the mail archive of the 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] Add DFA-based pipeline descriptions for MIPS 24K core.

Thanks for the patch.

David Ung <> writes:
> This patch adds support for the MIPS 24K processor family.
> The new attrib "cnv_mode" is introduced to catch the different float/int
> conversions, which have different delays.

I'm not too keen on the way you're overloading this attribute for xfer
as well as fcvt.  For instance, you change the movsi pattern like this:

> *************** beq\t%2,%.,1b\;\
> *** 3207,3212 ****
> --- 3231,3237 ----
>     { return mips_output_move (operands[0], operands[1]); }
>     [(set_attr "type"   "arith,const,const,load,store,fmove,xfer,fpload,xfer,fpstore,xfer,xfer,mthilo,xfer,load,xfer,store")
>      (set_attr "mode"   "SI")
> +    (set_attr "cnv_mode"       "*,*,*,*,*,*,I2S,*,F2I,*,F2I,I2S,*,*,*,*,*")
>      (set_attr "length" "4,*,*,*,*,4,4,*,4,*,4,4,4,4,*,4,*")])
>   (define_insn "*movsi_mips16"

but the moves aren't really "integer->float" or "float->integer".  As
far as gcc is concerned, both the source and destination are integer
values.  In other words, for xfer, you're using "integer" to mean
"in an integer register" whereas for fcvt you're using it to mean
"an integer value in a floating-point register".

If I've read your pipeline description correctly, you just want a way
of distinguishing coprocessor 1 transfers from coprocessor 0 transfers.
I'm not sure how much that will matter in practice (does anyone actually
use gcc's coprocessor 0 extensions?) but if you do want to keep them
separate, please just split "xfer" into two attribute values.  I realise
that involves changing more code, but I think it's the right thing to do.
Extra points for submitting the split as a separate patch.

I agree that it's a good idea to add an auxillary attribute for fcvt itself.

> + ;; I2S          integer to float single (SI/DI to SF)
> + ;; I2D          integer to float double (SI/DI to DF)
> + ;; F2I          float to integer (SF/DF to SI/DI)
> + ;; D2S          double to float single
> + ;; S2D          float single to double

F2I seems a bit of an odd-one-out here.  Since you're having to visit all
the fcvt patterns anyway, I'd prefer it if the attribute started off with
symmetric values, which would mean splitting F2I into D2I and S2I.

A couple of other minor niglets:

> + #define TARGET_MIPS24K              (mips_arch == PROCESSOR_24K)
> + #define TARGET_MIPS24KX             (mips_arch == PROCESSOR_24KX)
> [...]
> + #define TUNE_MIPS24K                (mips_tune == PROCESSOR_24K)
> + #define TUNE_MIPS24KX               (mips_tune == PROCESSOR_24KX)

You don't use these macros.  There's no point defining them until you do.

> (define_insn_reservation "r24k_int_load" 2 (and (eq_attr "cpu" "24k,24kx")
> 						(and (eq_attr "type" "load,prefetch")
> 						     (eq_attr "mode" "!SF,DF")))
>   	"r24k_iss+r24k_ixu_arith")

Watch the formatting and long lines.  Most of the other MIPS pipeline
descriptions have a different style (see and for examples).
It would be nice if you followed that here too.

> ; 2. Arithmetic: add, addi, addiu, addiupc, addu, and, andi, clo, clz,
> ;    ext, ins, lui, movn, movz, nor, or, ori, rotr, rotrv, seb, seh, sll, 
> ;    sllv, slt, slti, sltiu, sltu, sra, srav, srl, srlv, sub, subu, wsbh, 
> ;    xor, xori

Start-of-line comments generally use ";;" rather than ";".

> ; mul - delivers result to gpr in 5 cycles
> ;(define_insn_reservation "r24k_int_mul3" 5 (and (eq_attr "cpu" "24k,24kx")
> ;					       (eq_attr "type" "imul3")) 
> ;	"r24k_iss+(r24k_mul3a|r24k_mul3b|r24k_mul3c)*5")

If you want to keep this in the initial commit, please add a comment
to say why it's disabled.

> ;; .......................................................................
> ;;
> ;; End of DFA-based pipeline description for 24k
> ;;
> ;; .......................................................................

No need for this sort of thing at the end of a file.


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