This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH] MIPS: microMIPS and MCU ASE instruction set support
- From: "Fu, Chao-Ying" <fu at mips dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>, "Richard Sandiford" <rdsandiford at googlemail dot com>
- Cc: <binutils at sourceware dot org>, "Catherine Moore" <clm at codesourcery dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>, <dan at codesourcery dot com>
- Date: Tue, 1 Jun 2010 15:45:36 -0700
- Subject: RE: [PATCH] MIPS: microMIPS and MCU ASE instruction set support
- References: <alpine.DEB.1.10.1005181806590.4023@tp.orcam.me.uk> <87y6fa9u3t.fsf@firetop.home> <alpine.DEB.1.10.1005302126240.5236@tp.orcam.me.uk>
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