RE: [PATCH] disas/nanomips: Convert nanoMIPS disassembler to C
On Fri, 29 Jul 2022 at 10:18, Peter Maydell wrote:
> ...
> Is it possible to break this down into smaller pieces so it isn't one single
> enormous 5000 line patch ?
>
> I guess partial conversion is likely to run into compilation difficulties
> mid-series; if so we could do "disable the disassembler; convert it; reenable
> it".
>
> The rationale here is code review -- reviewing a single huge patch is
> essentially impossible, so it needs to be broken down into coherent smaller
> pieces to be reviewable.
Hi Peter,
Something like 90% of the patch is purely mechanical transformations of which
I've excerpted two examples below. (There are 3-4 chunks of mechanical
transformations, of which I've shown the most complicated type that accounts
for ~60% of the changed lines.) Did you intend to review this by having a
human inspect all of these?
Would it be feasible instead to provide a sed script to effect the mechanical
parts of the patch (the reviewer could review the script, apply it themselves
to verify equivalence if desired, and spot check the result) together with a
(much smaller) non-mechanical diff?
-Vince
@@ -2004,17 +1740,17 @@ std::string NMD::ADD_S(uint64 instruction)
* rt -
* rs -
*/
-std::string NMD::ADDIU_32_(uint64 instruction)
+static char *ADDIU_32_(uint64 instruction)
{
uint64 rt_value = extract_rt_25_24_23_22_21(instruction);
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
uint64 u_value = extract_u_15_to_0(instruction);
-std::string rt = GPR(copy(rt_value));
-std::string rs = GPR(copy(rs_value));
-std::string u = IMMEDIATE(copy(u_value));
+const char *rt = GPR(copy_ui(rt_value));
+const char *rs = GPR(copy_ui(rs_value));
+const char *u = IMMEDIATE_UI(copy_ui(u_value));
-return img::format("ADDIU %s, %s, %s", rt, rs, u);
+return img_format("ADDIU %s, %s, %s", rt, rs, u);
}
@@ -2027,15 +1763,15 @@ std::string NMD::ADDIU_32_(uint64 instruction)
* rt -
* rs -
*/
-std::string NMD::ADDIU_48_(uint64 instruction)
+static char *ADDIU_48_(uint64 instruction)
{
uint64 rt_value = extract_rt_41_40_39_38_37(instruction);
int64 s_value = extract_s__se31_15_to_0_31_to_16(instruction);
-std::string rt = GPR(copy(rt_value));
-std::string s = IMMEDIATE(copy(s_value));
+const char *rt = GPR(copy_ui(rt_value));
+const char *s = IMMEDIATE_I(copy_i(s_value));
-return img::format("ADDIU %s, %s", rt, s);
+return img_format("ADDIU %s, %s", rt, s);
}
RE: What to do with the nanomips disassembler (was: [PATCH] disas: Remove libvixl disassembler)
On Thu, Jun 9, 2022 at 10:34AM, Thomas Huth wrote: > On 09/06/2022 16.15, Claudio Fontana wrote: >> On 6/9/22 13:27, Claudio Fontana wrote: >>> On 6/9/22 10:57, Daniel P. Berrangé wrote: On Thu, Jun 09, 2022 at 10:47:24AM +0200, Thomas Huth wrote: > On 08/06/2022 17.51, Paolo Bonzini wrote: >> On 6/3/22 19:35, Thomas Huth wrote: >>> On 03/06/2022 19.26, Claudio Fontana wrote: > [...] maybe something we can now drop if there are no more C++ users? >>> >>> I thought about that, too, but we still have disas/nanomips.cpp >>> left and the Windows-related files in qga/vss-win32/* . >> >> That is pure C++ so it does not need the extra complication of >> "detect whether the C and C++ compiler are ABI-compatible" >> (typically due to different libasan/libtsan implementation between >> gcc and clang). So it's really just nanoMIPS that's left. > > Ok, so the next theoretical question is: If we get rid of the > nanomips.cpp file or convert it to plain C, would we then simplify > the code in configure again (and forbid C++ for the main QEMU > code), or would we rather keep the current settings in case we want > to re-introduce more C++ code again in the future? > It doesn't feel very compelling to have just 1 source file that's C++ in QEMU. I'm curious how we ended up with this nanomips.cpp file - perhaps it originated from another project that was C++ based ? The code itself doesn't look like it especially needs to be using C++. There's just 1 class there and every method is associated with that class, and external entry point from the rest of QEMU is just one boring method. Feels like it could easily have been done in C. Personally I'd prefer it to be converted to C, and if we want to add any C++ in future it should be justified & debated on its merits, rather than as an artifact of any historical artifacts such as the code in configure happening to still exist. >>> >>> I'll take a look at it, maybe I can turn it to C fairly quickly. >> >> It seems to be generated code, getting the original authors involved in the >> thread. > > Not sure whether the original mips folks are still around ... but the folks > from MediaTek recently expressed their interest in nanoMIPS: > > > https://lore.kernel.org/qemu-devel/20220504110403.613168-8-stefan.pe...@syrmia.com/ > > Maybe they could help with the nanoMIPS disassembler? > > I know it's likely a lot of work, but the best solution would maybe be to add > nanoMIPS support to capstone instead, then other projects could benefit from > the support in this library, too... > > If I googled that right, there is a LLVM implementation of nanoMIPS available > here: > > > https://github.com/milos1397/nanomips-outliner/tree/master/llvm/lib/Target/Mips__;!!CTRNKA9wMg0ARbw!ypaF-7vGkOBh5G3xybGwIuJdGpUfrMavQlYF_4T9iocgw5x994tABF_B_RsCJIdqY4vwVzA$ > > > ... so maybe that could be used as input for capstone (which is based on > llvm)? > > Thomas Yes, we are working on an LLVM port for nanoMIPS. It's functionally complete and pretty usable, although we're still tuning performance. The more official location is https://github.com/MediaTek-Labs/llvm-project. However, for now we're still using the binutils assembler; there's no encoding information in the current LLVM description. We have tentative plans to work on encoding and integrated-as later this year. Correct me if I'm wrong, but I would assume that, before that's available, it's not feasible to use capstone? Regardless, I think we can look at converting the existing disassembler from C++ to C. That would address the current concern, right? -Vince
RE: [PATCH] Revert "elf: Relax MIPS' elf_check_arch() to accept EM_NANOMIPS too"
RE: [PATCH v2] Revert "target/mips: Deprecate nanoMIPS ISA"
On Thursday, April 8, 2021 2:17 PM, Richard Henderson wrote: > NACK, for the reasons stated against v1: > https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg00663.html On Tuesday, April 6, 2021 11:21 AM, Richard Henderson wrote: > I think we should retain the deprecation until you actually follow through > with any of the upstreaming. > > You didn't even bother to commit your changes to a code repository -- merely > uploaded tarballs. There have been no posts to the > gcc mailing lists about > nanomips. > > A mere code dump is not active development. Maybe not, but we are in fact actively developing. :-) You’re right we haven’t published the source repos on github yet. It's been on our list. Maybe not today/tomorrow, but it'll definitely be done by next week. The nanoMIPS toolchain we inherited is based on gcc 6.3. We’ve been working on upgrading to gcc trunk since late February, but it's not a trivial task. As soon as we're done (hopefully before the summer), we'll propose the changes to the gcc mailing list. For now, we don't have many topics for the gcc lists, although https://gcc.gnu.org/pipermail/gcc/2021-March/235082.html is from our work. We've also started an LLVM port (https://github.com/MediaTek-Labs/llvm-project) as I mentioned in my previous message (https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09764.html). In sum, we're investing in open source nanoMIPS tools because it's an important technology for us, and QEMU is one of the key projects we want to have nanoMIPS supported in. -Vince Del Vecchio Compiler Team Lead & Deputy Director, DSP Core Technology, MediaTek
RE: [PATCH-for-5.2] target/mips: Deprecate nanoMIPS ISA
[Sorry for the resend; original seems to have been corrupted] On 11/2/20 12:27 PM, Philippe Mathieu-Daudé wrote: > The nanoMIPS ISA has been announced in 2018 for various projects: > > GCC: https://gcc.gnu.org/legacy-ml/gcc/2018-05/msg00012.html > Linux: https://lwn.net/Articles/753605/ > QEMU: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg530721.html > > Unfortunately the links referenced doesn't work anymore (www.mips.com). > > From this Wayback machine link [1] we can get to a working place to > download a toolchain (a more recent release than the one referenced in > the announcement mails): > http://codescape.mips.com/components/toolchain/nanomips/2018.04-02/dow > nloads.html > > ... > > Our deprecation policy do not allow feature removal before 2 release, > therefore declare the nanoMIPS ISA code deprecated as of QEMU 5.2. > This gives time to developers to update the QEMU community, or > interested parties to step in to maintain this code. Hi Philippe & everyone, Apologies for the late response. MediaTek is using the nanoMIPS architecture and is now doing nanoMIPS toolchain development. I believe Wave/MIPS are not any longer, so you can probably say we are taking over nanoMIPS toolchain development. We have just published a new release of the toolchain at https://github.com/MediaTek-Labs/nanomips-gnu-toolchain/releases/tag/nanoMIPS-2021.02-01 and we have started work on upgrading the toolchain to the latest versions in preparation for upstreaming. (We are also investigating an LLVM port.) We are also willing to act as maintainers for the QEMU nanoMIPS port, and we have agreement with the current and former MIPS QEMU maintainers (Aleksandar Rikalo & Aleksandar Markovic, both from Syrmia) to help us in this process if needed. To sum up, nanoMIPS is alive and in active development, so based on your criteria it should remain supported in QEMU. Please let us know how else we can help with this process. -Vince Del Vecchio Compiler Team Lead & Deputy Director, DSP Core Technology MediaTek, Inc.
