This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][MIPS] P5600 scheduling
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Jaydeep Patil <Jaydeep dot Patil at imgtec dot com>
- Cc: Rich Fuhler <Rich dot Fuhler at imgtec dot com>, Matthew Fortune <Matthew dot Fortune at imgtec dot com>, "gcc-patches\ at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Sun, 25 May 2014 11:48:02 +0100
- Subject: Re: [PATCH][MIPS] P5600 scheduling
- Authentication-results: sourceware.org; auth=none
- References: <BD7773622145634B952E5B54ACA8E34936A97AF1 at PUMAIL01 dot pu dot imgtec dot org> <87r43pjx4f dot fsf at talisman dot default> <BD7773622145634B952E5B54ACA8E34936A981D8 at PUMAIL01 dot pu dot imgtec dot org>
Jaydeep Patil <Jaydeep.Patil@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