RE: [PATCH][MIPS] P5600 scheduling

[Jaydeep Patil <Jaydeep dot Patil at imgtec dot com>]

Hi Richard,

Please refer to the attached patch files.

gcc-p5600-noMSA.patch
  The patch implements P5600 pipeline and scheduling for GP and FPU instructions.

gcc-p5600-noMSA-msched-weight.patch
  The patch implements -msched-weight option.

The -msched-weight option:
We are using ~650 hot-spot functions from VP9/VP8/H264/MPEG4 codes 
available to us as a test suite. The default Haifa-scheduler worked 
well for most of the functions, but excess spilling was observed in 
cases where register pressure was more than ~20. The 
-fsched-pressure flag proved good in some cases, but the algorithm 
focuses more on reducing register pressure.
We observed increase in stalls (but less spilling) with the -fsched- 
pressure option. When the register pressure goes beyond a certain 
threshold, the -msched-weight option tries to keep it down by 
promoting certain instructions up in the order. It has been 
implemented as part of TARGET_SCHED_REORDER target hook (function 
mips_sched_weight). The change is generic and there is nothing 
specific to MIPS.

When the register pressure goes beyond 15 then an instruction with 
maximum forward dependency is promoted ahead of the instruction at
READY[NREADY-1].
Scheduling of an INSN with maximum forward dependency enables early 
scheduling of instructions dependent on it.

When the register pressure goes beyond 25 and if consumer of the 
instruction in question (INSN) has higher priority than the 
instruction at READY[NREADY-1] then INSN is promoted. This chooses 
an INSN which has a high priority instruction dependent on it. This 
triggers the scheduling of that consumer instruction as early as 
possible to free up the registers used by that instruction.

Change log:

2014-05-21  Jaydeep Patil  <Jaydeep.Patil@imgtec.com>
	    Prachi Godbole  <Prachi.Godbole@imgtec.com>

	* config/mips/mips.c (mips_sched_weight): New function.
	* config/mips/mips.opt: New option -msched-weight.

2014-05-20  Jaydeep Patil  <Jaydeep.Patil@imgtec.com>
	    Prachi Godbole  <Prachi.Godbole@imgtec.com>

	* config/mips/p5600.md: New file.
	* config/mips/mips-cpus.def: Added p5600.
	* config/mips/mips-protos.h (mips_fmadd_bypass): Add prototype.
	* config/mips/mips.c (mips_fmadd_bypass): New function.
	* config/mips/mips.h: Added p5600 default settings.
	* config/mips/mips.md: Included p5600.md.

Regards,
Jaydeep

-----Original Message-----
From: Richard Sandiford [mailto:rdsandiford@googlemail.com] 
Sent: 20 May 2014 AM 02:37
To: Jaydeep Patil
Cc: Rich Fuhler; Matthew Fortune
Subject: Re: [PATCH][MIPS] P5600 scheduling

Jaydeep Patil <Jaydeep.Patil@imgtec.com> writes:
> Please refer to the attached patch which implements P5600 pipeline and 
> scheduling for GP and FPU instructions.
>
> Target option -msched-pressure has been implemented to reduce register 
> pressure while keeping the latency intact.

Please could you submit the -msched-pressure stuff separately?
Also please explain the heuristic in more detail.  Is it really MIPS-specific, or should it be done in generic scheduler code?

> +;; The address generation queue (AGQ) has AL2, CTISTD and LDSTA pipes 
> +(define_cpu_unit "p5600_agq, p5600_al2, p5600_ctistd, p5600_ldsta,
> +				p5600_gpdiv" "p5600_agen_pipe")

Please indent to the column after the ".

> +;; fadd, fsub
> +(define_insn_reservation "fpu_fadd" 4
> +  (eq_attr "type" "fadd,fabs,fneg")
> +  "p5600_fpu_long, p5600_fpu_apu")

The insn reservations should have a p5600_ prefix too.

> +;; fload
> +(define_insn_reservation "fpu_fload" 8
> +  (and (eq_attr "type" "fpload,fpidxload")
> +    (eq_attr "mode" "!TI"))
> +  "p5600_fpu_long, p5600_fpu_apu")
> +
> +;; fstore
> +(define_insn_reservation "fpu_fstore" 1
> +  (and (eq_attr "type" "fpstore,fpidxstore")
> +    (eq_attr "mode" "!TI"))
> +  "p5600_fpu_short, p5600_fpu_apu")

Checking for just !TI (without TF) seems a bit odd.  If you want to test for a single load or store, you should be able to use (eq_attr "insn_count" "1") instead.

> +;; call
> +(define_insn_reservation "int_call" 2
> +  (eq_attr "jal" "indirect,direct")
> +  "p5600_agq_ctistd")
> +

Very minor nit, but no blank line at the end of the file.

> @@ -13059,6 +13093,32 @@ mips_output_division (const char *division, rtx *operands)
>    return s;
>  }
>  

> +/* Return true if destination of IN_INSN is used as add source in
> +   OUT_INSN. Both IN_INSN and OUT_INSN are of type fmadd. Example:
> +   madd.s dst, x, y, z
> +   madd.s a, dst, b, c */
> +
> +bool
> +mips_fmadd_bypass (rtx out_insn, rtx in_insn) {
> +  int dst_reg, src_reg;
> +
> +  if ((recog_memoized (in_insn) < 0)
> +	  || (recog_memoized (out_insn) < 0))
> +    return false;

Style nit: just a single pair of brackets around the "if" condition.

Although if you take these insns being fmadds as a precondition, you should be able to gcc_assert instead.

> +  if (dst_reg == src_reg)
> +	return true;

Only indent this return by 4 spaces.

> @@ -13194,7 +13254,8 @@ mips_issue_rate (void)
>      case PROCESSOR_LOONGSON_2E:
>      case PROCESSOR_LOONGSON_2F:
>      case PROCESSOR_LOONGSON_3A:
> -      return 4;
> +	case PROCESSOR_P5600:
> +	  return 4;
>  
>      case PROCESSOR_XLP:
>        return (reload_completed ? 4 : 3); @@ -13329,6 +13390,9 @@ 
> mips_multipass_dfa_lookahead (void)
>    if (TUNE_OCTEON)
>      return 2;
>  
> +  if (TUNE_P5600)
> +	return 4;
> +

The indentation is a bit off here too.

> @@ -1050,6 +1051,7 @@
>    (eq_attr "type" "ghost")
>    "nothing")
>  
> +(include "P5600.md")
>  (include "4k.md")
>  (include "5k.md")
>  (include "20kc.md")

Please use lower-case "p" in the filename.

Otherwise the non-msched-pressure parts look good to me.  Obviously I'm only reviewing the surface details here, since I know nothing about the p5600 microarchitecture. :-)

It was difficult to review the -msched-weight part without more description.
Note that it had quite a few coding-standard violations though.

Thanks,
Richard

Attachment: gcc-p5600-noMSA.patch
Description: gcc-p5600-noMSA.patch

Attachment: gcc-p5600-noMSA-msched-weight.patch
Description: gcc-p5600-noMSA-msched-weight.patch