RE: [PATCH][MIPS] P5600 scheduling

[Jaydeep Patil]

Hi Richard,

Please refer to the attached patch files.

gcc-p5600-noMSA.patch 
TARGET_P5600 has been removed

gcc-p5600-noMSA-msched-weight.patch
Per-instruction structure has been created to store both GP and VEC pressures. 
We are using size of a mode to differentiate GP and VEC registers. Registers of 
size 16bytes and above are considered as vector registers.

Regards,
Jaydeep

-----Original Message-----
From: Richard Sandiford [mailto:rdsandif...@googlemail.com] 
Sent: 25 May 2014 PM 04:18
To: Jaydeep Patil
Cc: Rich Fuhler; Matthew Fortune; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH][MIPS] P5600 scheduling

Jaydeep Patil <jaydeep.pa...@imgtec.com> writes:
> 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.

This patch is OK, thanks.  Generally (i.e. when I remember to check) we don't 
define TARGET_* options until they're needed, so please leave out the 
TARGET_P5600 definition.

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

> +/* Return 1 if size of REG_MODE matches size of REG_TYPE.  */
> +#define GET_REGTYPE_SIZE(REG_MODE, REG_TYPE)                 \
> +  (((REG_TYPE) == GPREG                                              \
> +    && GET_MODE_SIZE ((REG_MODE)) <= GET_MODE_SIZE (DImode)) \
> +   || ((REG_TYPE) == VECREG                                  \
> +       && GET_MODE_SIZE ((REG_MODE)) > GET_MODE_SIZE (DImode)))

Is size the best way of distinguishing between these?  I would have thought 
that SFmode values would be counted against the VEC/FP register file instead.  
Also, in 32-bit mode a DImode would take up 2 registers rather than 1.  Maybe 
it would be better to count SCALAR_INT_MODE_P modes against GPREG and others 
against VECREG.

That would still mishandle things like float<->integer conversions.
We could avoid that by checking the register classes in the instruction 
constraints, but since this is a heuristic, in practice it might be OK to 
ignore corner cases like that.

Rather than:

> +       INSN_REGTYPE_WEIGHT (insn, GPREG)
> +         = find_insn_regtype_weight (insn, GPREG);
> +       INSN_REGTYPE_WEIGHT (insn, VECREG)
> +         = find_insn_regtype_weight (insn, VECREG);

and:

> +       if (regtype_weight[GPREG] != NULL)
> +         CURR_REGTYPE_PRESSURE (GPREG)
> +           += INSN_REGTYPE_WEIGHT (insn, GPREG);
> +       if (regtype_weight[VECREG] != NULL)
> +         CURR_REGTYPE_PRESSURE (VECREG)
> +           += INSN_REGTYPE_WEIGHT (insn, VECREG);

I think it would be more efficient to have a per-insn structure that contains 
both the GP and VEC pressures and only process each instruction once.

> +  regtype_weight[GPREG] = (short *) xcalloc (old_max_uid, sizeof 
> + (short));  regtype_weight[VECREG] = (short *) xcalloc (old_max_uid, 
> + sizeof (short));

... = XCNEWVEC (short, old_max_uid);

> +  FOR_EACH_BB_REVERSE_FN (b, cfun)
> +    {
> +      rtx insn, next_tail, head, tail;
> +      get_ebb_head_tail (b, b, &head, &tail);
> +      next_tail = NEXT_INSN (tail);
> +      for (insn = head; insn != next_tail; insn = NEXT_INSN (insn))
> +     {
> +       /* Handle register life information.  */
> +       if (!INSN_P (insn))
> +         continue;

AFAICT this loop doesn't carry any state between instructions, so it should be:

  FOR_EACH_BB_FN (b, cfun)
    FOR_BB_INSNS (b, insn)
      if (NONDEBUG_INSN_P (insn))
        {
          ...
        }

Thanks,
Richard

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

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