This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Add DFA-based pipeline descriptions for MIPS 24K core.
- From: Richard Sandiford <rsandifo at redhat dot com>
- To: David Ung <davidu at mips dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 04 May 2005 15:34:09 +0100
- Subject: Re: [patch] Add DFA-based pipeline descriptions for MIPS 24K core.
- References: <1115206984.1620.89.camel@localhost.localdomain>
Thanks for the patch.
David Ung <davidu@mips.com> 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.
However:
> + ;; 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 9000.md and sb1.md 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.
Richard