Re: [PATCH][MIPS] P5600 scheduling

[Richard Sandiford <rdsandiford at googlemail dot com>]

Jaydeep Patil <Jaydeep.Patil@imgtec.com> writes:
> Please refer to the attached patch files.
>
> gcc-p5600-noMSA.patch 
> TARGET_P5600 has been removed

Sorry, noticed one other thing:

> +bool
> +mips_fmadd_bypass (rtx out_insn, rtx in_insn)
> +{
> +  int dst_reg, src_reg;
> +  
> +  gcc_assert (get_attr_type (in_insn) == TYPE_FMADD);
> +  gcc_assert (get_attr_type (out_insn) == TYPE_FMADD);
> +
> +  if (recog_memoized (in_insn) < 0
> +      || recog_memoized (out_insn) < 0)
> +    return false;

What I meant with the assertions vs. recog_memoized is that
get_attr_type (...) == TYPE_FMADD only holds if the instructions
are recognisable.  Once you've asserted that the types are correct,
the following:

  if (recog_memoized (in_insn) < 0
      || recog_memoized (out_insn) < 0)
    return false;

is dead code.  The patch is OK with the recog_memoized calls removed.
No need to repost the patch; just commit it with that change once
the copyright log-jam is sorted out.

> 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.

But what I was trying to say is that that seems like the wrong distinction.
AIUI the P5600 has an FPU and the VEC registers are overlaid on the FP
registers.  So if we have a floating-point mode smaller than 16 bytes,
shouldn't it be counted against VEC rather than GP?  That's why I was
suggesting using SCALAR_INT_MODE_P (mode) ? GP : VEC.

Obviously the only thing that matters here is what gives the best code.
But if counting FP stuff against GP rather than VEC gives better code,
it'd be interesting to understand why.

Thanks,
Richard

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@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.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