This is the mail archive of the binutils@sourceware.org mailing list for the binutils 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: [PATCH] MIPS: microMIPS and MCU ASE instruction set support


Maciej W. Rozycki wrote:
>  I'm placing notes throughout and I'll be asking people for 
> explanations 
> where applicable.  Chao-ying, would you please look into the 
> few pieces of 
> code below I am a bit uncertain about?

  Yes.

> 
> > > 	(A_BFD_RELOC_HI16_S, A_BFD_RELOC_HI16, A_BFD_RELOC_LO16): New
> > > 	relocation wrapper macros.
> > > 	(A_BFD_RELOC_GPREL16): Likewise.
> > > 	(A_BFD_RELOC_MIPS_GOT16, A_BFD_RELOC_MIPS_GOT_HI16): Likewise.
> > > 	(A_BFD_RELOC_MIPS_GOT_LO16, A_BFD_RELOC_MIPS_HIGHEST): Likewise.
> > > 	(A_BFD_RELOC_MIPS_HIGHER, A_BFD_RELOC_MIPS_GOT_DISP): Likewise.
> > > 	(A_BFD_RELOC_MIPS_GOT_PAGE, A_BFD_RELOC_MIPS_GOT_OFST): 
> Likewise.
> > > 	(A_BFD_RELOC_MIPS_SUB, A_BFD_RELOC_MIPS_JALR): Likewise.
> > 
> > Did you consider doing the translation from non-microMIPS to
> > microMIPS in the macro_* functions, rather than in their callers?
> > I fear it'll be too easy to accidentally forget to use 
> A_BFD_* in future.
> 
>  Agreed, I didn't like the new macros from the very 
> beginning.  Chao-ying 
> -- any thoughts?

  I am fine.  But the new method may be bigger than the A_BFD* method.


> 
> > > 	(macro_start, macro_warning, macro_end): Likewise.
> > 
> > +  else if ((subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
> > +	   || (subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND))
> > +    return _("Macro instruction of the wrong size in a 
> branch delay slot"
> > +	     " that requires a 16-bit or 32-bit instruction");
> > 
> > Did you consider adding a flag to distinguish the 32-bit 
> and 16-bit cases?
> > It'd be nice to be consistent with the non-relaxed error if 
> possible.
> 
>  Chao-ying?  I've had a look actually and flag may not be 
> necessary to get 
> the functionality.  I'm fixing this up elsewhere already.

  I am fine with this functionality. Maybe passing one more parameter to
macro_warning() can help to distinguish two cases.

> 
> > +  /* If either one implementation contains one 
> instruction, we need to check
> > +     the delay slot size requirement.  */
> > +  if (mips_macro_warning.num_insns[0] == 1
> > +      || mips_macro_warning.num_insns[1] == 1)
> > +    {
> > +      if (mips_macro_warning.num_insns[0] == 
> mips_macro_warning.num_insns[1]
> > +	  && mips_macro_warning.sizes[0] == mips_macro_warning.sizes[1])
> > +	{
> > +	  /* Either the macro has a single implementation or both
> > +	     implementations are 1 instruction with the same size.
> > +	     Emit the warning now.  */
> > +	  if ((mips_macro_warning.delay_slot_16bit_p
> > +	       && mips_macro_warning.sizes[0] != 2)
> > +	      || (mips_macro_warning.delay_slot_32bit_p
> > +		  && mips_macro_warning.sizes[0] != 4))
> > +	    {
> > +	      const char *msg;
> > +	      msg = macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST);
> > +	      if (msg != 0)
> > +		as_warn (msg);
> > +	    }
> > +	}
> > +      else
> > +	{
> > +	  relax_substateT subtype;
> > +
> > +	  /* Set up the relaxation warning flags.  */
> > +	  subtype = 0;
> > +	  if (mips_macro_warning.delay_slot_16bit_p)
> > +	    {
> > +	      if (mips_macro_warning.num_insns[0] != 1
> > +		  || mips_macro_warning.sizes[0] != 2)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > +	      if (mips_macro_warning.num_insns[1] != 1
> > +		  || mips_macro_warning.sizes[1] != 2)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > +	    }
> > +	  if (mips_macro_warning.delay_slot_32bit_p)
> > +	    {
> > +	      if (mips_macro_warning.num_insns[0] != 1
> > +		  || mips_macro_warning.sizes[0] != 4)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > +	      if (mips_macro_warning.num_insns[1] != 1
> > +		  || mips_macro_warning.sizes[1] != 4)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > +	    }
> > +
> > +	  /* One implementation might need a warning but the other
> > +	     definitely doesn't.  */
> > +	  mips_macro_warning.first_frag->fr_subtype |= subtype;
> > +	}
> > +    }
> > 
> > Why not work out the subtype, then check whether both 
> ERROR_FIRST and
> > ERROR_SECOND are set?
> 
>  Chao-ying?

  Do you mean this kind of code?
Ex:
  if (mips_macro_warning.num_insns[0] == 1
      || mips_macro_warning.num_insns[1] == 1)
    {
      if (mips_macro_warning.delay_slot_16bit_p)
        {
          if (mips_macro_warning.num_insns[0] != 1
              || mips_macro_warning.sizes[0] != 2)
            subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
          if (mips_macro_warning.num_insns[1] != 1
              || mips_macro_warning.sizes[1] != 2)
            subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
        }
      else if (mips_macro_warning.delay_slot_32bit_p)
        {
          if (mips_macro_warning.num_insns[0] != 1
              || mips_macro_warning.sizes[0] != 4)
            subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
          if (mips_macro_warning.num_insns[1] != 1
              || mips_macro_warning.sizes[1] != 4)
            subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
        }

      if ((subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
           && (subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND))
        {
          const char *msg;
          msg = macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST); //
NEED TO PASS 16-bit or 32-bit info
          if (msg != 0)
             as_warn (msg);
        }
      else
        {
          /* One implementation might need a warning but the other
             definitely doesn't.  */
          mips_macro_warning.first_frag->fr_subtype |= subtype;
        }
    }

> 
> > +	  if (mips_macro_warning.delay_slot_p)
> > +	    {
> > +	      if (mips_macro_warning.num_insns[0] > 1)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_FIRST;
> > +	      if (mips_macro_warning.num_insns[1] > 1)
> > +		subtype |= RELAX_DELAY_SLOT_SIZE_ERROR_SECOND;
> > +	    }
> > 
> > I don't get why this hunk is needed.  I thought ERROR_FIRST 
> and ERROR_SECOND
> > controlled cases where a macro has a single-insn expansion 
> that is the
> > wrong size, which ought to be handled by the block above.  
> If the code
> > really is needed, you should add a comment explaining why.
> 
>  Chao-ying?


  I agree.  The delay_slot_p check is duplicated, and we can discard it
for
ERROR_FIRST and ERROR_SECOND when num_insns[]>1.  My old code
double-checked the condition for 
number of instructions in delay slots.

> 
> > > 	(macro_build): Likewise.
> > 
> > +  if (mips_opts.micromips)
> > +    {
> > +      if (strcmp (name, "lui") == 0)
> > +	micromips_macro_build (ep, name, "s,u", args);
> > +      else if (strcmp (fmt, "d,w,<") == 0)
> > +	micromips_macro_build (ep, name, "t,r,<", args);
> > +      else
> > +	micromips_macro_build (ep, name, fmt, args);
> > +      va_end (args);
> > +      return;
> > +    }
> > 
> > A bit of commentary might help explain the letter switch here.
> 
>  Chao-ying?

  Because I didn't change all the code before calling LUI macro for
microMIPS, I need
to magically change the operand string for microMIPS to use "s,u" for
microMIPS LUI.
"d,w,<" is another case that we need to map it to "t,r,<" for microMIPS.
If we can search all the places and replace all calls with correct
operand strings for microMIPS,
this special code can be dropped.

> 
> > > 	(macro_build_jalr): Likewise.
> > 
> > +  if (mips_opts.micromips)
> > +    {
> > +      if (HAVE_NEWABI)
> > +	macro_build (NULL, "jalr", "t,s", RA, PIC_CALL_REG);
> > +      else
> > +	macro_build (NULL, "jalr", "mj", PIC_CALL_REG);
> > +    }
> > +  else
> > +    macro_build (NULL, "jalr", "d,s", RA, PIC_CALL_REG);
> > 
> > Why HAVE_NEWABI?  Do you want a 32-bit insn for R_MIPS_JALR?
> 
>  Chao-ying?

  OK.  This code is done before I put the R_MIPS_JALR patch into GAS and
LD.
The new code is as follows.
Ex:
static void
macro_build_jalr (expressionS *ep)
{
  char *f = NULL;

  if (MIPS_JALR_HINT_P (ep))
    {
      frag_grow (8);
      f = frag_more (0);
    }
  if (mips_opts.micromips)
    macro_build (NULL, "jalr", "t,s", RA, PIC_CALL_REG);
  else
    macro_build (NULL, "jalr", "d,s", RA, PIC_CALL_REG);
  if (MIPS_JALR_HINT_P (ep))
    fix_new_exp (frag_now, f - frag_now->fr_literal,
                 4, ep, FALSE, A_BFD_RELOC_MIPS_JALR); // THIS MAY BE
FIXED BY A NEW METHOD.
}

  And, we need to modify elfxx-mips.c to support
BFD_RELOC_MICROMIPS_JALR to convert jalr to bal for microMIPS.

> 
> > If so, you should check MIPS_JALR_HINT_P (ep) instead.
> 
>  I may have missed that while updating the change for the 
> recent JALR hint 
> support, let me see...
> 

> > > 	(md_convert_frag): Likewise.
> > 
> > -      /* Possibly emit a warning if we've chosen the 
> longer option.  */
> > -      if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
> > -	  == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
> > +      if (!(fragp->fr_subtype & RELAX_USE_SECOND))
> > +  	{
> > +	  /* Check if the size in branch delay slot is ok.  */
> > +	  if (fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_FIRST)
> > +	    {
> > +	      const char *msg = macro_warning (fragp->fr_subtype);
> > +	      if (msg != 0)
> > +		as_warn_where (fragp->fr_file, fragp->fr_line, msg);
> > +	    }
> > +	}
> > +      else
> >  	{
> > -	  const char *msg = macro_warning (fragp->fr_subtype);
> > -	  if (msg != 0)
> > -	    as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg);
> > +	  /* Check if the size in branch delay slot is ok.
> > +	     Possibly emit a warning if we've chosen the longer 
> option.  */
> > +	  if ((fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND)
> > +	      || (fragp->fr_subtype & RELAX_SECOND_LONGER))
> > +	    {
> > +	      const char *msg = macro_warning (fragp->fr_subtype);
> > +	      if (msg != 0)
> > +		as_warn_where (fragp->fr_file, fragp->fr_line, msg);
> > +	    }
> >  	}
> > 
> > This doesn't accurately preserve the previous:
> > 
> >       if (((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
> > 	  == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
> > 
> > behaviour.
> 
>  Chao-ying?

  How about this?

Ex:

      if ((((fragp->fr_subtype & RELAX_USE_SECOND) != 0)
           == ((fragp->fr_subtype & RELAX_SECOND_LONGER) != 0))
          || (!(fragp->fr_subtype & RELAX_USE_SECOND)
              && (fragp->fr_subtype &
RELAX_DELAY_SLOT_SIZE_ERROR_FIRST))
          || ((fragp->fr_subtype & RELAX_USE_SECOND)
              && (fragp->fr_subtype &
RELAX_DELAY_SLOT_SIZE_ERROR_SECOND)))
        {
          const char *msg = macro_warning (fragp->fr_subtype); // MAY
NEED TO PASS 16-bit or 32-bit info
          if (msg != 0)
            as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg);
        }



> 
> > > 	(micromips_ip): New function.
> > 
> > +      /* Try to search "16" or "32" in the str.  */
> > +      if ((t = strstr (str, "16")) != NULL && t < save_s)
> > +	{
> > +	  /* Make sure "16" is before the first '.' if '.' exists.  */
> > +	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > +
> > +	  /* Make sure "16" is at the end of insn name, if no '.'.  */
> > +	  if ((s = strchr (str, '.')) == NULL
> > +	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > +
> > +	  micromips_16 = TRUE;
> > +	  for (s = t + 2; *s != '\0'; ++s)
> > +	    *(s - 2) = *s;
> > +	  *(s - 2) = '\0';
> > +
> > +	  for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
> > +	    continue;
> > +
> > +	  if (ISSPACE (*s))
> > +	    {
> > +	      save_c = *s;
> > +	      *s++ = '\0';
> > +	    }
> > +
> > +	  if ((insn = (struct mips_opcode *) hash_find 
> (micromips_op_hash, str))
> > +	      == NULL)
> > +	    {
> > +	      int i;
> > +	      int length;
> > +	      micromips_16 = FALSE;
> > +
> > +	      /* Restore the character we overwrite above (if any).  */
> > +	      if (save_c)
> > +		*(--s) = save_c;
> > +
> > +	      length = strlen (str);
> > +	      for (i = length - 1; &str[i] >= t; i--)
> > +		{
> > +		  str[i + 2] = str[i];
> > +		  if (t == &str[i])
> > +		    {
> > +		      str[i + 1] = '6';
> > +		      str[i] = '1';
> > +		      str[length + 2] = '\0';
> > +		      break;
> > +		    }
> > +		}
> > +
> > +	      insn_error = "unrecognized 16-bit version of 
> microMIPS opcode";
> > +	      return;
> > +	    }
> > +	}
> > +      else if ((t = strstr (str, "32")) != NULL && t < save_s)
> > +	{
> > +	  /* For some instruction names, we already have 32, so we need
> > +	     to seek the second 32 to process.  Ex: bposge3232, 
> dsra3232.  */
> > +	  char *new_t;
> > +	  if ((new_t = strstr (t + 2, "32")) != NULL && new_t < save_s)
> > +	    t = new_t;
> > +
> > +	  /* Make sure "32" is before the first '.' if '.' exists.  */
> > +	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > +
> > +	  /* Make sure "32" is at the end of the name, if no '.'.  */
> > +	  if ((s = strchr (str, '.')) == NULL
> > +	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > +
> > +	  micromips_32 = TRUE;
> > +	  for (s = t + 2; *s != '\0'; ++s)
> > +	    *(s - 2) = *s;
> > +	  *(s - 2) = '\0';
> > +
> > +	  for (s = t; *s != '\0' && !ISSPACE (*s); ++s)
> > +	    continue;
> > +
> > +	  if (ISSPACE (*s))
> > +	    {
> > +	      save_c = *s;
> > +	      *s++ = '\0';
> > +	    }
> > +
> > +	  if ((insn = (struct mips_opcode *) hash_find 
> (micromips_op_hash, str))
> > +	      == NULL)
> > +	    {
> > +	      int i;
> > +	      int length;
> > +	      micromips_32 = FALSE;
> > +
> > +	      /* Restore the character we overwrite above (if any).  */
> > +	      if (save_c)
> > +		*(--s) = save_c;
> > +
> > +	      length = strlen (str);
> > +	      for (i = length - 1; &str[i] >= t; i--)
> > +		{
> > +		  str[i + 2] = str[i];
> > +		  if (t == &str[i])
> > +		    {
> > +		      str[i + 1] = '2';
> > +		      str[i] = '3';
> > +		      str[length + 2] = '\0';
> > +		      break;
> > +		    }
> > +		}
> > +
> > +	      insn_error = "unrecognized 32-bit version of 
> microMIPS opcode";
> > +	      return;
> > +	    }
> > 
> > Far too much cut-&-paste between the "16" and "32" cases.  Also:
> > 
> > +      if ((t = strstr (str, "16")) != NULL && t < save_s)
> > 
> > t < save_s must surely be true, since save_s is the null terminator.

  Yes.  I used t < save_s, because I don't know save_s points to NULL at
this point.
We can discard save_s now.

> > 
> > +	  /* Make sure "16" is before the first '.' if '.' exists.  */
> > +	  if ((s = strchr (str, '.')) != NULL && (t + 2 != s))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > +
> > +	  /* Make sure "16" is at the end of insn name, if no '.'.  */
> > +	  if ((s = strchr (str, '.')) == NULL
> > +	      && (!ISSPACE (*(t + 2)) && *(t + 2) != '\0'))
> > +	    {
> > +	      insn_error = "unrecognized opcode";
> > +	      return;
> > +	    }
> > 
> > Don't call strchr (str, '.') twice like that.  Better would be:
> > 
> > 	s = strchr (str, '.');

  Yes.

> > 
> > followed by the two checks.  Isn't the ISSPACE check 
> redundant though,
> > given that you've terminated the string at the first space?  I would
> > have thought:
> > 
> >         if (t + 2 != (s ? s : save_s))
> > 
> > would be enough.  Errors should start with a capital 
> letter.  Missing
> > internationalisation.
> 
>  Chao-ying?

  Yes. "if (t + 2 != (s ? s : save_s))" is enough.

> 
> > You could use alloca to create an opcode without the "16" or "32",
> > which would make the error-reporting code simpler.  It's best not
> > to change the user's source line if we can help it.
> 
>  Agreed.

  Yes.

> 
> > +	      if (!insn_error)
> > +		{
> > +		  static char buf[100];
> > +		  sprintf (buf,
> > +			   _("opcode not supported on this 
> processor: %s (%s)"),
> > +			   mips_cpu_info_from_arch 
> (mips_opts.arch)->name,
> > +			   mips_cpu_info_from_isa 
> (mips_opts.isa)->name);
> > +		  insn_error = buf;
> > +		}
> > +	      if (save_c)
> > +		*(--s) = save_c;
> > +
> > +	      if (micromips_16 || micromips_32)
> > +		{
> > +		  int i;
> > +		  int length;
> > +
> > +		  length = strlen (str);
> > +		  for (i = length - 1; i >= 0; i--)
> > +		    {
> > +		      str[i + 2] = str[i];
> > +		      if (t == &str[i])
> > +			break;
> > +		    }
> > +		  if (micromips_16)
> > +		    {
> > +		      insn_error =
> > +			"unrecognized 16-bit version of 
> microMIPS opcode";
> > +		      str[i + 1] = '6';
> > +		      str[i] = '1';
> > +		    }
> > +		  else
> > +		    {
> > +		      insn_error =
> > +			"unrecognized 32-bit version of 
> microMIPS opcode";
> > +		      str[i + 1] = '2';
> > +		      str[i] = '3';
> > +		    }
> > +		  str[length + 2] = '\0';
> > +		}
> > +	      return;
> > 
> > Why override the insn_error unconditionally like this?  E.g.:
> > 
> > 	jar16	$30,$26
> > 
> >     Error: unrecognized 16-bit version of microMIPS opcode 
> `jar16 $30,$26'
> > 
> > implies there's a 32-bit opcode.  I'd also have thought that the
> > "opcode not supported on this processor" would triumph if 
> it applies.
> 
>  The error you've seen comes from the previous hunk above 
> rather than this 
> one which I think is unnecessary code duplication.  It's all rather 
> over-complicated and I'm working on getting it polished.  
> I've fixed this 
> piece of code:
> 
> 	.set	micromips
> 	.set	noreorder
> 	bltzall	$2, bar
> 	 addiusp 256
> 
> producing this nonsense:
> 
> bltzall.s: Assembler messages:
> bltzall.s:4: Error: opcode not supported on this processor: 
> mips1 (mips1) `addiusp 256'
> 
> too.  Also the original loop seems ill-formed to me, with 
> most of code 
> intended to be executed at most once, after the loop's terminating 
> condition triggered -- i.e. that shouldn't be in the loop in 
> the first 
> place.
> 

  What code in the loop do you refer to?  I am not clear.

  Thanks for updating the patch!

Regards,
Chao-ying


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