From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2457 invoked by alias); 1 Jun 2010 22:47:00 -0000 Received: (qmail 2445 invoked by uid 22791); 1 Jun 2010 22:46:54 -0000 X-SWARE-Spam-Status: No, hits=-1.0 required=5.0 tests=AWL,BAYES_05,TW_FX,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from dns1.mips.com (HELO dns1.mips.com) (63.167.95.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 01 Jun 2010 22:46:00 +0000 Received: from MTVEXCHANGE.mips.com ([192.168.36.60]) by dns1.mips.com (8.13.8/8.13.8) with ESMTP id o51MjmYe014778; Tue, 1 Jun 2010 15:45:49 -0700 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH] MIPS: microMIPS and MCU ASE instruction set support Date: Tue, 01 Jun 2010 22:47:00 -0000 Message-ID: <94BD67F8AF3ED34FA362C662BA1F12C502BB5F91@MTVEXCHANGE.mips.com> In-Reply-To: References: <87y6fa9u3t.fsf@firetop.home> From: "Fu, Chao-Ying" To: "Maciej W. Rozycki" , "Richard Sandiford" Cc: , "Catherine Moore" , "Joseph S. Myers" , X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2010-06/txt/msg00019.txt.bz2 Maciej W. Rozycki wrote: > I'm placing notes throughout and I'll be asking people for=20 > explanations=20 > where applicable. Chao-ying, would you please look into the=20 > few pieces of=20 > code below I am a bit uncertain about? Yes. >=20 > > > (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):=20 > Likewise. > > > (A_BFD_RELOC_MIPS_SUB, A_BFD_RELOC_MIPS_JALR): Likewise. > >=20 > > 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=20 > A_BFD_* in future. >=20 > Agreed, I didn't like the new macros from the very=20 > beginning. Chao-ying=20 > -- any thoughts? I am fine. But the new method may be bigger than the A_BFD* method. >=20 > > > (macro_start, macro_warning, macro_end): Likewise. > >=20 > > + 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=20 > branch delay slot" > > + " that requires a 16-bit or 32-bit instruction"); > >=20 > > Did you consider adding a flag to distinguish the 32-bit=20 > and 16-bit cases? > > It'd be nice to be consistent with the non-relaxed error if=20 > possible. >=20 > Chao-ying? I've had a look actually and flag may not be=20 > necessary to get=20 > 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. >=20 > > + /* If either one implementation contains one=20 > instruction, we need to check > > + the delay slot size requirement. */ > > + if (mips_macro_warning.num_insns[0] =3D=3D 1 > > + || mips_macro_warning.num_insns[1] =3D=3D 1) > > + { > > + if (mips_macro_warning.num_insns[0] =3D=3D=20 > mips_macro_warning.num_insns[1] > > + && mips_macro_warning.sizes[0] =3D=3D 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] !=3D 2) > > + || (mips_macro_warning.delay_slot_32bit_p > > + && mips_macro_warning.sizes[0] !=3D 4)) > > + { > > + const char *msg; > > + msg =3D macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST); > > + if (msg !=3D 0) > > + as_warn (msg); > > + } > > + } > > + else > > + { > > + relax_substateT subtype; > > + > > + /* Set up the relaxation warning flags. */ > > + subtype =3D 0; > > + if (mips_macro_warning.delay_slot_16bit_p) > > + { > > + if (mips_macro_warning.num_insns[0] !=3D 1 > > + || mips_macro_warning.sizes[0] !=3D 2) > > + subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_FIRST; > > + if (mips_macro_warning.num_insns[1] !=3D 1 > > + || mips_macro_warning.sizes[1] !=3D 2) > > + subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_SECOND; > > + } > > + if (mips_macro_warning.delay_slot_32bit_p) > > + { > > + if (mips_macro_warning.num_insns[0] !=3D 1 > > + || mips_macro_warning.sizes[0] !=3D 4) > > + subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_FIRST; > > + if (mips_macro_warning.num_insns[1] !=3D 1 > > + || mips_macro_warning.sizes[1] !=3D 4) > > + subtype |=3D 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 |=3D subtype; > > + } > > + } > >=20 > > Why not work out the subtype, then check whether both=20 > ERROR_FIRST and > > ERROR_SECOND are set? >=20 > Chao-ying? Do you mean this kind of code? Ex: if (mips_macro_warning.num_insns[0] =3D=3D 1 || mips_macro_warning.num_insns[1] =3D=3D 1) { if (mips_macro_warning.delay_slot_16bit_p) { if (mips_macro_warning.num_insns[0] !=3D 1 || mips_macro_warning.sizes[0] !=3D 2) subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_FIRST; if (mips_macro_warning.num_insns[1] !=3D 1 || mips_macro_warning.sizes[1] !=3D 2) subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_SECOND; } else if (mips_macro_warning.delay_slot_32bit_p) { if (mips_macro_warning.num_insns[0] !=3D 1 || mips_macro_warning.sizes[0] !=3D 4) subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_FIRST; if (mips_macro_warning.num_insns[1] !=3D 1 || mips_macro_warning.sizes[1] !=3D 4) subtype |=3D 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 =3D macro_warning (RELAX_DELAY_SLOT_SIZE_ERROR_FIRST); // NEED TO PASS 16-bit or 32-bit info if (msg !=3D 0) as_warn (msg); } else { /* One implementation might need a warning but the other definitely doesn't. */ mips_macro_warning.first_frag->fr_subtype |=3D subtype; } } >=20 > > + if (mips_macro_warning.delay_slot_p) > > + { > > + if (mips_macro_warning.num_insns[0] > 1) > > + subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_FIRST; > > + if (mips_macro_warning.num_insns[1] > 1) > > + subtype |=3D RELAX_DELAY_SLOT_SIZE_ERROR_SECOND; > > + } > >=20 > > I don't get why this hunk is needed. I thought ERROR_FIRST=20 > and ERROR_SECOND > > controlled cases where a macro has a single-insn expansion=20 > that is the > > wrong size, which ought to be handled by the block above.=20=20 > If the code > > really is needed, you should add a comment explaining why. >=20 > 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=20 number of instructions in delay slots. >=20 > > > (macro_build): Likewise. > >=20 > > + if (mips_opts.micromips) > > + { > > + if (strcmp (name, "lui") =3D=3D 0) > > + micromips_macro_build (ep, name, "s,u", args); > > + else if (strcmp (fmt, "d,w,<") =3D=3D 0) > > + micromips_macro_build (ep, name, "t,r,<", args); > > + else > > + micromips_macro_build (ep, name, fmt, args); > > + va_end (args); > > + return; > > + } > >=20 > > A bit of commentary might help explain the letter switch here. >=20 > 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. >=20 > > > (macro_build_jalr): Likewise. > >=20 > > + 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); > >=20 > > Why HAVE_NEWABI? Do you want a 32-bit insn for R_MIPS_JALR? >=20 > 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 =3D NULL; if (MIPS_JALR_HINT_P (ep)) { frag_grow (8); f =3D 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. >=20 > > If so, you should check MIPS_JALR_HINT_P (ep) instead. >=20 > I may have missed that while updating the change for the=20 > recent JALR hint=20 > support, let me see... >=20 > > > (md_convert_frag): Likewise. > >=20 > > - /* Possibly emit a warning if we've chosen the=20 > longer option. */ > > - if (((fragp->fr_subtype & RELAX_USE_SECOND) !=3D 0) > > - =3D=3D ((fragp->fr_subtype & RELAX_SECOND_LONGER) !=3D 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 =3D macro_warning (fragp->fr_subtype); > > + if (msg !=3D 0) > > + as_warn_where (fragp->fr_file, fragp->fr_line, msg); > > + } > > + } > > + else > > { > > - const char *msg =3D macro_warning (fragp->fr_subtype); > > - if (msg !=3D 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=20 > option. */ > > + if ((fragp->fr_subtype & RELAX_DELAY_SLOT_SIZE_ERROR_SECOND) > > + || (fragp->fr_subtype & RELAX_SECOND_LONGER)) > > + { > > + const char *msg =3D macro_warning (fragp->fr_subtype); > > + if (msg !=3D 0) > > + as_warn_where (fragp->fr_file, fragp->fr_line, msg); > > + } > > } > >=20 > > This doesn't accurately preserve the previous: > >=20 > > if (((fragp->fr_subtype & RELAX_USE_SECOND) !=3D 0) > > =3D=3D ((fragp->fr_subtype & RELAX_SECOND_LONGER) !=3D 0)) > >=20 > > behaviour. >=20 > Chao-ying? How about this? Ex: if ((((fragp->fr_subtype & RELAX_USE_SECOND) !=3D 0) =3D=3D ((fragp->fr_subtype & RELAX_SECOND_LONGER) !=3D 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 =3D macro_warning (fragp->fr_subtype); // MAY NEED TO PASS 16-bit or 32-bit info if (msg !=3D 0) as_warn_where (fragp->fr_file, fragp->fr_line, "%s", msg); } >=20 > > > (micromips_ip): New function. > >=20 > > + /* Try to search "16" or "32" in the str. */ > > + if ((t =3D strstr (str, "16")) !=3D NULL && t < save_s) > > + { > > + /* Make sure "16" is before the first '.' if '.' exists. */ > > + if ((s =3D strchr (str, '.')) !=3D NULL && (t + 2 !=3D s)) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > > + > > + /* Make sure "16" is at the end of insn name, if no '.'. */ > > + if ((s =3D strchr (str, '.')) =3D=3D NULL > > + && (!ISSPACE (*(t + 2)) && *(t + 2) !=3D '\0')) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > > + > > + micromips_16 =3D TRUE; > > + for (s =3D t + 2; *s !=3D '\0'; ++s) > > + *(s - 2) =3D *s; > > + *(s - 2) =3D '\0'; > > + > > + for (s =3D t; *s !=3D '\0' && !ISSPACE (*s); ++s) > > + continue; > > + > > + if (ISSPACE (*s)) > > + { > > + save_c =3D *s; > > + *s++ =3D '\0'; > > + } > > + > > + if ((insn =3D (struct mips_opcode *) hash_find=20 > (micromips_op_hash, str)) > > + =3D=3D NULL) > > + { > > + int i; > > + int length; > > + micromips_16 =3D FALSE; > > + > > + /* Restore the character we overwrite above (if any). */ > > + if (save_c) > > + *(--s) =3D save_c; > > + > > + length =3D strlen (str); > > + for (i =3D length - 1; &str[i] >=3D t; i--) > > + { > > + str[i + 2] =3D str[i]; > > + if (t =3D=3D &str[i]) > > + { > > + str[i + 1] =3D '6'; > > + str[i] =3D '1'; > > + str[length + 2] =3D '\0'; > > + break; > > + } > > + } > > + > > + insn_error =3D "unrecognized 16-bit version of=20 > microMIPS opcode"; > > + return; > > + } > > + } > > + else if ((t =3D strstr (str, "32")) !=3D NULL && t < save_s) > > + { > > + /* For some instruction names, we already have 32, so we need > > + to seek the second 32 to process. Ex: bposge3232,=20 > dsra3232. */ > > + char *new_t; > > + if ((new_t =3D strstr (t + 2, "32")) !=3D NULL && new_t < save_s) > > + t =3D new_t; > > + > > + /* Make sure "32" is before the first '.' if '.' exists. */ > > + if ((s =3D strchr (str, '.')) !=3D NULL && (t + 2 !=3D s)) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > > + > > + /* Make sure "32" is at the end of the name, if no '.'. */ > > + if ((s =3D strchr (str, '.')) =3D=3D NULL > > + && (!ISSPACE (*(t + 2)) && *(t + 2) !=3D '\0')) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > > + > > + micromips_32 =3D TRUE; > > + for (s =3D t + 2; *s !=3D '\0'; ++s) > > + *(s - 2) =3D *s; > > + *(s - 2) =3D '\0'; > > + > > + for (s =3D t; *s !=3D '\0' && !ISSPACE (*s); ++s) > > + continue; > > + > > + if (ISSPACE (*s)) > > + { > > + save_c =3D *s; > > + *s++ =3D '\0'; > > + } > > + > > + if ((insn =3D (struct mips_opcode *) hash_find=20 > (micromips_op_hash, str)) > > + =3D=3D NULL) > > + { > > + int i; > > + int length; > > + micromips_32 =3D FALSE; > > + > > + /* Restore the character we overwrite above (if any). */ > > + if (save_c) > > + *(--s) =3D save_c; > > + > > + length =3D strlen (str); > > + for (i =3D length - 1; &str[i] >=3D t; i--) > > + { > > + str[i + 2] =3D str[i]; > > + if (t =3D=3D &str[i]) > > + { > > + str[i + 1] =3D '2'; > > + str[i] =3D '3'; > > + str[length + 2] =3D '\0'; > > + break; > > + } > > + } > > + > > + insn_error =3D "unrecognized 32-bit version of=20 > microMIPS opcode"; > > + return; > > + } > >=20 > > Far too much cut-&-paste between the "16" and "32" cases. Also: > >=20 > > + if ((t =3D strstr (str, "16")) !=3D NULL && t < save_s) > >=20 > > 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. > >=20 > > + /* Make sure "16" is before the first '.' if '.' exists. */ > > + if ((s =3D strchr (str, '.')) !=3D NULL && (t + 2 !=3D s)) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > > + > > + /* Make sure "16" is at the end of insn name, if no '.'. */ > > + if ((s =3D strchr (str, '.')) =3D=3D NULL > > + && (!ISSPACE (*(t + 2)) && *(t + 2) !=3D '\0')) > > + { > > + insn_error =3D "unrecognized opcode"; > > + return; > > + } > >=20 > > Don't call strchr (str, '.') twice like that. Better would be: > >=20 > > s =3D strchr (str, '.'); Yes. > >=20 > > followed by the two checks. Isn't the ISSPACE check=20 > redundant though, > > given that you've terminated the string at the first space? I would > > have thought: > >=20 > > if (t + 2 !=3D (s ? s : save_s)) > >=20 > > would be enough. Errors should start with a capital=20 > letter. Missing > > internationalisation. >=20 > Chao-ying? Yes. "if (t + 2 !=3D (s ? s : save_s))" is enough. >=20 > > 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. >=20 > Agreed. Yes. >=20 > > + if (!insn_error) > > + { > > + static char buf[100]; > > + sprintf (buf, > > + _("opcode not supported on this=20 > processor: %s (%s)"), > > + mips_cpu_info_from_arch=20 > (mips_opts.arch)->name, > > + mips_cpu_info_from_isa=20 > (mips_opts.isa)->name); > > + insn_error =3D buf; > > + } > > + if (save_c) > > + *(--s) =3D save_c; > > + > > + if (micromips_16 || micromips_32) > > + { > > + int i; > > + int length; > > + > > + length =3D strlen (str); > > + for (i =3D length - 1; i >=3D 0; i--) > > + { > > + str[i + 2] =3D str[i]; > > + if (t =3D=3D &str[i]) > > + break; > > + } > > + if (micromips_16) > > + { > > + insn_error =3D > > + "unrecognized 16-bit version of=20 > microMIPS opcode"; > > + str[i + 1] =3D '6'; > > + str[i] =3D '1'; > > + } > > + else > > + { > > + insn_error =3D > > + "unrecognized 32-bit version of=20 > microMIPS opcode"; > > + str[i + 1] =3D '2'; > > + str[i] =3D '3'; > > + } > > + str[length + 2] =3D '\0'; > > + } > > + return; > >=20 > > Why override the insn_error unconditionally like this? E.g.: > >=20 > > jar16 $30,$26 > >=20 > > Error: unrecognized 16-bit version of microMIPS opcode=20 > `jar16 $30,$26' > >=20 > > implies there's a 32-bit opcode. I'd also have thought that the > > "opcode not supported on this processor" would triumph if=20 > it applies. >=20 > The error you've seen comes from the previous hunk above=20 > rather than this=20 > one which I think is unnecessary code duplication. It's all rather=20 > over-complicated and I'm working on getting it polished.=20=20 > I've fixed this=20 > piece of code: >=20 > .set micromips > .set noreorder > bltzall $2, bar > addiusp 256 >=20 > producing this nonsense: >=20 > bltzall.s: Assembler messages: > bltzall.s:4: Error: opcode not supported on this processor:=20 > mips1 (mips1) `addiusp 256' >=20 > too. Also the original loop seems ill-formed to me, with=20 > most of code=20 > intended to be executed at most once, after the loop's terminating=20 > condition triggered -- i.e. that shouldn't be in the loop in=20 > the first=20 > place. >=20 What code in the loop do you refer to? I am not clear. Thanks for updating the patch! Regards, Chao-ying