[committed] Fix MIPS p5600 scheduler

The p5600 scheduler wasn't restricting itself to -mtune=p5600 and so
was being used for other CPUs too.  This showed up as a failure in
various tests, including gcc.target/mips/octeon-pipe-1.c.  (Thinking
about it, it was probably also why umips-lwp-*.c started failing,
although the patch I just committed is still OK after this fix.)

Guys: please make sure you do a before-and-after comparison of test results,
even if it "obviously" shouldn't be necessary.  This amount of fallout
in gcc.target/mips would have been a red flag that something was wrong.

Tested on mips64-linux-gnu and applied.

Thanks,
Richard


gcc/
	* config/mips/p5600.md: Add missing cpu tests.

Hi Richard,

Thanks for fixing this. I'm afraid I managed to get confused between
failures we had seen sporadically in our development work and thought
they were known regressions on trunk waiting to be fixed when actually
we were introducing them.

Apologies for the breakage.

Regards,
Matthew

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: 17 July 2014 21:18
> To: gcc-patches@gcc.gnu.org
> Cc: Jaydeep Patil; Matthew Fortune
> Subject: [committed] Fix MIPS p5600 scheduler
> 
> The p5600 scheduler wasn't restricting itself to -mtune=p5600 and so
> was being used for other CPUs too.  This showed up as a failure in
> various tests, including gcc.target/mips/octeon-pipe-1.c.  (Thinking
> about it, it was probably also why umips-lwp-*.c started failing,
> although the patch I just committed is still OK after this fix.)
> 
> Guys: please make sure you do a before-and-after comparison of test results,
> even if it "obviously" shouldn't be necessary.  This amount of fallout
> in gcc.target/mips would have been a red flag that something was wrong.
> 
> Tested on mips64-linux-gnu and applied.
> 
> Thanks,
> Richard
> 
> 
> gcc/
> 	* config/mips/p5600.md: Add missing cpu tests.
> 
> Index: gcc/config/mips/p5600.md
> ===================================================================
> --- gcc/config/mips/p5600.md	2014-07-17 20:53:50.423095856 +0100
> +++ gcc/config/mips/p5600.md	2014-07-17 20:53:50.764100479 +0100
> @@ -47,52 +47,62 @@ (define_reservation "p5600_alq_alu" "p56
> 
>  ;; fadd, fsub
>  (define_insn_reservation "p5600_fpu_fadd" 4
> -  (eq_attr "type" "fadd,fabs,fneg")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fadd,fabs,fneg"))
>    "p5600_fpu_long, p5600_fpu_apu")
> 
>  ;; fabs, fneg, fcmp
>  (define_insn_reservation "p5600_fpu_fabs" 2
> -  (eq_attr "type" "fabs,fneg,fcmp,fmove")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fabs,fneg,fcmp,fmove"))
>    "p5600_fpu_short, p5600_fpu_apu")
> 
>  ;; fload
>  (define_insn_reservation "p5600_fpu_fload" 8
> -  (eq_attr "type" "fpload,fpidxload")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fpload,fpidxload"))
>    "p5600_fpu_long, p5600_fpu_apu")
> 
>  ;; fstore
>  (define_insn_reservation "p5600_fpu_fstore" 1
> -  (eq_attr "type" "fpstore,fpidxstore")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fpstore,fpidxstore"))
>    "p5600_fpu_short, p5600_fpu_apu")
> 
>  ;; fmadd
>  (define_insn_reservation "p5600_fpu_fmadd" 9
> -  (eq_attr "type" "fmadd")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fmadd"))
>    "p5600_fpu_long, p5600_fpu_apu")
> 
>  ;; fmul
>  (define_insn_reservation "p5600_fpu_fmul" 5
> -  (eq_attr "type" "fmul")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fmul"))
>    "p5600_fpu_long, p5600_fpu_apu")
> 
>  ;; fdiv, fsqrt
>  (define_insn_reservation "p5600_fpu_div" 17
> -  (eq_attr "type" "fdiv,frdiv,fsqrt,frsqrt")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fdiv,frdiv,fsqrt,frsqrt"))
>    "p5600_fpu_long, p5600_fpu_apu*17")
> 
>  ;; fcvt
>  (define_insn_reservation "p5600_fpu_fcvt" 4
> -  (eq_attr "type" "fcvt")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "fcvt"))
>    "p5600_fpu_long, p5600_fpu_apu")
> 
>  ;; mtc
>  (define_insn_reservation "p5600_fpu_fmtc" 7
> -  (eq_attr "type" "mtc")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "mtc"))
>    "p5600_fpu_short, p5600_fpu_store")
> 
>  ;; mfc
>  (define_insn_reservation "p5600_fpu_fmfc" 4
> -  (eq_attr "type" "mfc")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "mfc"))
>    "p5600_fpu_short, p5600_fpu_store")
> 
>  ;; madd/msub feeding into the add source
> @@ -105,100 +115,120 @@ (define_bypass 5 "p5600_fpu_fmadd" "p560
> 
>  ;; and
>  (define_insn_reservation "p5600_int_and" 1
> -  (eq_attr "move_type" "logical")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "move_type" "logical"))
>    "p5600_alq_alu")
> 
>  ;; lui
>  (define_insn_reservation "p5600_int_lui" 1
> -  (eq_attr "move_type" "const")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "move_type" "const"))
>    "p5600_alq_alu")
> 
>  ;; Load lb, lbu, lh, lhu, lq, lw, lw_i2f, lwxs
>  (define_insn_reservation "p5600_int_load" 4
> -  (eq_attr "move_type" "load")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "move_type" "load"))
>    "p5600_agq_ldsta")
> 
>  ;; store
>  (define_insn_reservation "p5600_int_store" 3
> -  (eq_attr "move_type" "store")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "move_type" "store"))
>    "p5600_agq_ldsta")
> 
>  ;; andi, sll, srl, seb, seh
>  (define_insn_reservation "p5600_int_arith_1" 1
> -  (eq_attr "move_type" "andi,sll0,signext")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "move_type" "andi,sll0,signext"))
>    "p5600_agq_al2 | p5600_alq_alu")
> 
>  ;; addi, addiu, ori, xori, add, addu
>  (define_insn_reservation "p5600_int_arith_2" 1
> -  (eq_attr "alu_type" "add,or,xor")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "alu_type" "add,or,xor"))
>    "p5600_agq_al2 | p5600_alq_alu")
> 
>  ;; nor, sub
>  (define_insn_reservation "p5600_int_arith_3" 1
> -  (eq_attr "alu_type" "nor,sub")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "alu_type" "nor,sub"))
>    "p5600_alq_alu")
> 
>  ;; srl, sra, rotr, slt, sllv, srlv
>  (define_insn_reservation "p5600_int_arith_4" 1
> -  (eq_attr "type" "shift,slt,move")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "shift,slt,move"))
>    "p5600_agq_al2 | p5600_alq_alu")
> 
>  ;; nop
>  (define_insn_reservation "p5600_int_nop" 0
> -  (eq_attr "type" "nop")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "nop"))
>    "p5600_agq_al2")
> 
>  ;; clo, clz
>  (define_insn_reservation "p5600_int_countbits" 1
> -  (eq_attr "type" "clz")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "clz"))
>    "p5600_agq_al2")
> 
>  ;; Conditional moves
>  (define_insn_reservation "p5600_int_condmove" 1
> -  (eq_attr "type" "condmove")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "condmove"))
>    "p5600_agq_al2")
> 
>  ;; madd, msub
>  (define_insn_reservation "p5600_dsp_mac" 5
> -  (eq_attr "type" "imadd")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "imadd"))
>    "p5600_agq_al2")
> 
>  ;; mfhi/lo
>  (define_insn_reservation "p5600_dsp_mfhilo" 1
> -  (eq_attr "type" "mfhi,mflo")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "mfhi,mflo"))
>    "p5600_agq_al2")
> 
>  ;; mthi/lo
>  (define_insn_reservation "p5600_dsp_mthilo" 5
> -  (eq_attr "type" "mthi,mtlo")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "mthi,mtlo"))
>    "p5600_agq_al2")
> 
>  ;; mult, multu, mul
>  (define_insn_reservation "p5600_dsp_mult" 5
> -  (eq_attr "type" "imul3,imul")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "imul3,imul"))
>    "p5600_agq_al2")
> 
>  ;; branch and jump
>  (define_insn_reservation "p5600_int_branch" 1
> -  (eq_attr "type" "branch,jump")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "branch,jump"))
>    "p5600_agq_ctistd")
> 
>  ;; prefetch
>  (define_insn_reservation "p5600_int_prefetch" 3
> -  (eq_attr "type" "prefetch,prefetchx")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "prefetch,prefetchx"))
>    "p5600_agq_ldsta")
> 
>  ;; divide
>  (define_insn_reservation "p5600_int_div" 8
> -  (eq_attr "type" "idiv")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "idiv"))
>    "p5600_agq_al2+p5600_gpdiv*8")
> 
>  ;; arith
>  (define_insn_reservation "p5600_int_arith_5" 2
> -  (eq_attr "type" "arith")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "type" "arith"))
>    "p5600_agq_al2")
> 
>  ;; call
>  (define_insn_reservation "p5600_int_call" 2
> -  (eq_attr "jal" "indirect,direct")
> +  (and (eq_attr "cpu" "p5600")
> +       (eq_attr "jal" "indirect,direct"))
>    "p5600_agq_ctistd")

On Jul 17, 2014, at 11:47 PM, Matthew Fortune <Matthew.Fortune@imgtec.com> wrote:
> Thanks for fixing this. I'm afraid I managed to get confused between
> failures we had seen sporadically in our development work and thought
> they were known regressions on trunk waiting to be fixed when actually
> we were introducing them.

I recommend contrib/compare_tests for checking for regressions.  It tells you just what you need to know, and if they first line is unimportant, you’re done reading.  If the first three lines are interesting, then you have at least 1 regression left.  By reducing the line count of what you have to look at, it making it exceedingly hard to ignore it and exceedingly hard to accidentally put in a regression.

> On Jul 17, 2014, at 11:47 PM, Matthew Fortune <Matthew.Fortune@imgtec.com>
> wrote:
> > Thanks for fixing this. I'm afraid I managed to get confused between
> > failures we had seen sporadically in our development work and thought
> > they were known regressions on trunk waiting to be fixed when actually
> > we were introducing them.
> 
> I recommend contrib/compare_tests for checking for regressions.  It tells
> you just what you need to know, and if they first line is unimportant,
> you're done reading.  If the first three lines are interesting, then you
> have at least 1 regression left.  By reducing the line count of what you
> have to look at, it making it exceedingly hard to ignore it and exceedingly
> hard to accidentally put in a regression.

Thanks Mike. The advice is appreciated.

Matthew