Re: [committed] R3000 scheduling description

[Richard Sandiford <rsandifo at redhat dot com>]

Eric Christopher <echristo@redhat.com> writes:
> Adds a description for the r3000 scheduling description. No noticable
> change in scheduling.

Can you be more specific?  Like: how did you test for differences?

I noticed a hell of a lot of changes in a c-torture assembly comparison.
Which isn't really surprising, since from my reading, this is far from
being a like-for-like change.

> "unknown,prefetch,prefetchx,condmove,mthilo,const,arith,shift,slt,clz,trap,fmove,fadd,fmadd,fabs,fneg,fcvt,fsqrt,frsqrt,multi,nop"))

Niggly detail, but please split the long line.  Newlines are allowed.

> +(define_insn_reservation "r3k_load_alu" 2
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "load, fpload, fpidxload, xfer"))
> +  "r3k_alu*2")

Another niggly detail, but please be consistent about whether the type
string has spaces or not.  Here it does, in others it doesn't.

More importantly, the original description had a separate memory unit,
but here you're using the ALU to do loads.  It really does make a
difference, especially when you're reserving units for more than one
cycle, as you're doing here.

For example, in the old description, an unrelated ALU operation could
issue immediately after a load.  In the new one, nothing can issue in
the cycle after a load.

> +(define_insn_reservation "r3k_call_alu" 2
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "branch,jump,call"))
> +  "r3k_alu*2")

Where did this come from?  I can't see any equivalent in the original version.

> +(define_insn_reservation "r3k_hilo_alu" 3
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "mfhilo"))
> +  "r3k_alu*3")

This isn't what the old scheduler said.  It said:

(define_function_unit "imuldiv"  1 0
  (eq_attr "type" "mthilo,mfhilo")
  1 3)

i.e. a ready-delay of 1 and an issue-delay of 3.  It also says that
mfhilo & mthilo use an integer multiplication/division unit,
not the ALU.

> +(define_insn_reservation "r3k_fcmp_alu" 2
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "fcmp, fadd"))
> +  "r3k_alu*2")

Here again you're using r3k_alu when the original used a different unit
(which it called "adder").

> +(define_insn_reservation "r3k_imul_alu" 12
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "imul, imadd"))
> +  "r3k_alu*12")
> +
> +(define_insn_reservation "r3k_idiv_alu" 35
> +  (and (eq_attr "cpu" "r3000")
> +       (eq_attr "type" "idiv"))
> +  "r3k_alu*35")

ALU rather "imuldiv" again.

> +(define_insn_reservation "r3k_fmul_single_alu" 4
> +  (and (eq_attr "cpu" "r3000")
> +       (and (eq_attr "type" "fmul")
> +	    (eq_attr "mode" "SF")))
> +  "r3k_alu*4")
> +
> +(define_insn_reservation "r3k_fmul_double_alu" 5
> +  (and (eq_attr "cpu" "r3000")
> +       (and (eq_attr "type" "fmul")
> +	    (eq_attr "mode" "DF")))
> +  "r3k_alu*5")

If the original description is to be believed, there's a separate
unit for floating-point multiplications ("mult").

> +(define_insn_reservation "r3k_fdiv_single_alu" 12
> +  (and (eq_attr "cpu" "r3000")
> +       (and (eq_attr "type" "fdiv")
> +	    (eq_attr "mode" "SF")))
> +  "r3k_alu*12")
> +
> +(define_insn_reservation "r3k_fdiv_double_alu" 19
> +  (and (eq_attr "cpu" "r3000")
> +       (and (eq_attr "type" "fdiv")
> +	    (eq_attr "mode" "DF")))
> +  "r3k_alu*19")

...and a separate unit again for division.

> -       (eq_attr "cpu" "!r3000,r3900,r4600,r4650,r4100,r4120,r4300,r5000"))
> +       (eq_attr "cpu" "r3900,r4600,r4650,r4100,r4120,r4300,r5000"))

Erm, surely you don't want to remove "!"?  Lots of other instances
in the patch as well.

Richard