This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: Fix debug info for MIPS doubles


Thanks for the patch.

Daniel Jacobowitz <drow@false.org> writes:
> +
> +static rtx
> +mips_dwarf_register_span (rtx reg)

Please add a comment saying "Implement TARGET_DWARF_REGISTER_SPAN.",
just so that it's obvious at first glance that this is a standard
target hook.

> +{
> +  rtx parts[4];
> +  int i, words;
> +  unsigned regno = REGNO (reg);
> +  enum machine_mode mode = GET_MODE (reg);
> +

Minor nit, but seeing as there are quite a few variables,
please initialise using separate assignments.

> +  /* By default, GCC emits register-sized pieces for each consecutive
> +     register.  But the low order bits are always in the lower numbered
> +     register in a MIPS FPU, so this is backwards for big endian.  */
> +  if (FP_REG_P (regno)
> +      && TARGET_BIG_ENDIAN
> +      && !TARGET_FLOAT64
> +      && !TARGET_SINGLE_FLOAT
> +      && FLOAT_MODE_P (mode)
> +      && mode != SFmode
> +      && mode != SCmode)
> +    {
> +      words = GET_MODE_SIZE (mode) / UNITS_PER_FPREG;
> +      gcc_assert (words % 2 == 0 && words <= 8);
> +
> +      for (i = 0; i < words; i++, regno++)
> +	parts[i ^ 1] = gen_rtx_REG (SFmode, regno);
> +
> +      return gen_rtx_PARALLEL (VOIDmode, gen_rtvec_v (words, parts));
> +    }

This condition seems overly complex.  Did you come across a case that
forced you to handle SCmode?  I wouldn't have expected to see a hard
SCmode FPR: the MIPS function hooks should return pairs of registers
instead, and SCmode variables ought to be CONCATs of two SFmode registers.
If we need to handle SCmode FPRs, we'd need to buff up other parts
of the port too.

Also, did you find a condition that needs words == 4 or words == 8?
Again, I wouldn't have expected that given the definition of
mips_hard_regno_mode_ok, and other parts of the port would probably need
to be looked at.  (The patch is inconsistent here; parts only supports
words <= 4, but the assert is for words <= 8.)

If the answer to both questions is "no", I'd suggest:

  enum machine_mode mode;

  /* By default, GCC maps increasing register numbers to increasing
     memory locations, but paired FPRs are always little-endian,
     regardless of the prevailing endianness.  */
  mode = GET_MODE (reg);
  if (FP_REG_P (REGNO (reg))
      && TARGET_BIG_ENDIAN
      && MAX_FPRS_PER_FMT > 1
      && GET_MODE_SIZE (mode) > UNITS_PER_FPREG)
    {
      gcc_assert (GET_MODE_SIZE (mode) == UNITS_PER_HWFPVALUE);
      high = mips_subword (reg, true);
      low = mips_subword (reg, false);
      return gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, high, low));
    }
  return NULL_RTX;

(untested).  Patch is OK with that change, if it works.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]