Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
On Fri, Mar 16, 2018 at 10:03 AM, Michael Clarkwrote: > > On Thu, Mar 15, 2018 at 12:27 PM, Peter Maydell > wrote: > >> On 10 March 2018 at 21:25, Philippe Mathieu-Daudé >> wrote: >> > On 03/09/2018 10:01 PM, Michael Clark wrote: >> >> Logic bug caused the string size calculation for the RISC-V >> >> format ISA string to be small. This fix allows slack for rv128. >> >> >> >> Cc: Palmer Dabbelt >> >> Cc: Peter Maydell >> >> Cc: Eric Blake >> >> Signed-off-by: Michael Clark >> >> --- >> >> target/riscv/cpu.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> >> index 4851890..1456535 100644 >> >> --- a/target/riscv/cpu.c >> >> +++ b/target/riscv/cpu.c >> >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { >> >> char *riscv_isa_string(RISCVCPU *cpu) >> >> { >> >> int i; >> >> -size_t maxlen = 5 + ctz32(cpu->env.misa); >> >> +size_t maxlen = 8 + ctpop64(cpu->env.misa); >> > >> > Can you document the magic 5/8? >> > >> > This looks nice, but this seems to me too much optimization to save few >> > bytes, using sizeof(riscv_exts) is overflow-free. >> > >> > Maybe this is enough and self-explanatory: >> > >> >const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts); >> > >> >> char *isa_string = g_new0(char, maxlen); >> >> snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); >> > >> > Also, if you keep the snprintf() return value, you can (naming it 'n') >> > simplify (also easier to review): >> > >> >> for (i = 0; i < sizeof(riscv_exts); i++) { >> >> >> > if (cpu->env.misa & RV(riscv_exts[i])) { >> > - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; >> > + isa_string[n++] = tolower(riscv_exts[i]); >> > } >> > } >> > >> > and simply use g_new() with: >> > >> > + isa_string[n] = '\0'; >> > >> > return isa_string; >> > } >> >> Hi -- any chance of a respin of this patch that addresses Philippe's >> review comments so we can fix it before rc0? This is causing >> my merge-build tests to fail about 50% of the time on OpenBSD >> at the moment... > > > I'll respin asap. > > I was out earlier this week as I was at the RISC-V Hackathon at the > Embedded Linux Conference in Portland. I also have to go through the review > backlog for the post merge spec conformance and cleanup series... > I've sent out the riscv_isa_string fix patch as well as the RISC-V Post-merge spec conformance and cleanup series independently. I could combine them so we have one post-merge PR that contains the riscv_isa_string string fix, the spec conformance fixes and cleanup. Many of the cleanup patches have been reviewed by Phil, but the patches that haven't been reviewed are spec conformance fixes and likely require a reading of the RISC-V Privileged ISA Specification, which may not happen, unless anyone with RISC-V knowledge has time. They have sign off. If I submit a PR it will include the checkpatch fixes that I noticed after sending the patches. The series has been tested pretty extensively and the earlier revision is what we have in the riscv.org repo and in SiFive's freedom-u-sdk. i.e. it is what folk are using. I've been working inside of QEMU RISC-V quite a lot with this series applied, running the latest Fedora image with SMP Linux, building QEMU inside of QEMU as I'm working on a TCG backend for RISC-V that I started this Monday during the RISC-V Hackathon. FYI: This is the work in progress branch on the TCG backend for RISC-V (it's not quite complete but the bones are there): - https://github.com/michaeljclark/riscv-qemu/tree/riscv-tcg-backend
Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
On Thu, Mar 15, 2018 at 12:27 PM, Peter Maydellwrote: > On 10 March 2018 at 21:25, Philippe Mathieu-Daudé wrote: > > On 03/09/2018 10:01 PM, Michael Clark wrote: > >> Logic bug caused the string size calculation for the RISC-V > >> format ISA string to be small. This fix allows slack for rv128. > >> > >> Cc: Palmer Dabbelt > >> Cc: Peter Maydell > >> Cc: Eric Blake > >> Signed-off-by: Michael Clark > >> --- > >> target/riscv/cpu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > >> index 4851890..1456535 100644 > >> --- a/target/riscv/cpu.c > >> +++ b/target/riscv/cpu.c > >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { > >> char *riscv_isa_string(RISCVCPU *cpu) > >> { > >> int i; > >> -size_t maxlen = 5 + ctz32(cpu->env.misa); > >> +size_t maxlen = 8 + ctpop64(cpu->env.misa); > > > > Can you document the magic 5/8? > > > > This looks nice, but this seems to me too much optimization to save few > > bytes, using sizeof(riscv_exts) is overflow-free. > > > > Maybe this is enough and self-explanatory: > > > >const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts); > > > >> char *isa_string = g_new0(char, maxlen); > >> snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); > > > > Also, if you keep the snprintf() return value, you can (naming it 'n') > > simplify (also easier to review): > > > >> for (i = 0; i < sizeof(riscv_exts); i++) { > >> > > if (cpu->env.misa & RV(riscv_exts[i])) { > > - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; > > + isa_string[n++] = tolower(riscv_exts[i]); > > } > > } > > > > and simply use g_new() with: > > > > + isa_string[n] = '\0'; > > > > return isa_string; > > } > > Hi -- any chance of a respin of this patch that addresses Philippe's > review comments so we can fix it before rc0? This is causing > my merge-build tests to fail about 50% of the time on OpenBSD > at the moment... I'll respin asap. I was out earlier this week as I was at the RISC-V Hackathon at the Embedded Linux Conference in Portland. I also have to go through the review backlog for the post merge spec conformance and cleanup series...
Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
On 10 March 2018 at 21:25, Philippe Mathieu-Daudéwrote: > On 03/09/2018 10:01 PM, Michael Clark wrote: >> Logic bug caused the string size calculation for the RISC-V >> format ISA string to be small. This fix allows slack for rv128. >> >> Cc: Palmer Dabbelt >> Cc: Peter Maydell >> Cc: Eric Blake >> Signed-off-by: Michael Clark >> --- >> target/riscv/cpu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 4851890..1456535 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { >> char *riscv_isa_string(RISCVCPU *cpu) >> { >> int i; >> -size_t maxlen = 5 + ctz32(cpu->env.misa); >> +size_t maxlen = 8 + ctpop64(cpu->env.misa); > > Can you document the magic 5/8? > > This looks nice, but this seems to me too much optimization to save few > bytes, using sizeof(riscv_exts) is overflow-free. > > Maybe this is enough and self-explanatory: > >const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts); > >> char *isa_string = g_new0(char, maxlen); >> snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); > > Also, if you keep the snprintf() return value, you can (naming it 'n') > simplify (also easier to review): > >> for (i = 0; i < sizeof(riscv_exts); i++) { >> > if (cpu->env.misa & RV(riscv_exts[i])) { > - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; > + isa_string[n++] = tolower(riscv_exts[i]); > } > } > > and simply use g_new() with: > > + isa_string[n] = '\0'; > > return isa_string; > } Hi -- any chance of a respin of this patch that addresses Philippe's review comments so we can fix it before rc0? This is causing my merge-build tests to fail about 50% of the time on OpenBSD at the moment... thanks -- PMM
Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
On 03/10/2018 10:25 PM, Philippe Mathieu-Daudé wrote: > On 03/09/2018 10:01 PM, Michael Clark wrote: >> Logic bug caused the string size calculation for the RISC-V >> format ISA string to be small. This fix allows slack for rv128. >> >> Cc: Palmer Dabbelt>> Cc: Peter Maydell >> Cc: Eric Blake >> Signed-off-by: Michael Clark >> --- >> target/riscv/cpu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 4851890..1456535 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { >> char *riscv_isa_string(RISCVCPU *cpu) >> { >> int i; >> -size_t maxlen = 5 + ctz32(cpu->env.misa); >> +size_t maxlen = 8 + ctpop64(cpu->env.misa); > > Can you document the magic 5/8? > > This looks nice, but this seems to me too much optimization to save few > bytes, using sizeof(riscv_exts) is overflow-free. > > Maybe this is enough and self-explanatory: > >const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts); Oops: const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1; > >> char *isa_string = g_new0(char, maxlen); >> snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); > > Also, if you keep the snprintf() return value, you can (naming it 'n') > simplify (also easier to review): > >> for (i = 0; i < sizeof(riscv_exts); i++) { >> > if (cpu->env.misa & RV(riscv_exts[i])) { > - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; > + isa_string[n++] = tolower(riscv_exts[i]); > } > } > > and simply use g_new() with: > > + isa_string[n] = '\0'; > > return isa_string; > } > > Regards, > > Phil. >
Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
On 03/09/2018 10:01 PM, Michael Clark wrote: > Logic bug caused the string size calculation for the RISC-V > format ISA string to be small. This fix allows slack for rv128. > > Cc: Palmer Dabbelt> Cc: Peter Maydell > Cc: Eric Blake > Signed-off-by: Michael Clark > --- > target/riscv/cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 4851890..1456535 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { > char *riscv_isa_string(RISCVCPU *cpu) > { > int i; > -size_t maxlen = 5 + ctz32(cpu->env.misa); > +size_t maxlen = 8 + ctpop64(cpu->env.misa); Can you document the magic 5/8? This looks nice, but this seems to me too much optimization to save few bytes, using sizeof(riscv_exts) is overflow-free. Maybe this is enough and self-explanatory: const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts); > char *isa_string = g_new0(char, maxlen); > snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); Also, if you keep the snprintf() return value, you can (naming it 'n') simplify (also easier to review): > for (i = 0; i < sizeof(riscv_exts); i++) { > if (cpu->env.misa & RV(riscv_exts[i])) { - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; + isa_string[n++] = tolower(riscv_exts[i]); } } and simply use g_new() with: + isa_string[n] = '\0'; return isa_string; } Regards, Phil.
[Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
Logic bug caused the string size calculation for the RISC-V format ISA string to be small. This fix allows slack for rv128. Cc: Palmer DabbeltCc: Peter Maydell Cc: Eric Blake Signed-off-by: Michael Clark --- target/riscv/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 4851890..1456535 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -391,7 +391,7 @@ static const TypeInfo riscv_cpu_type_info = { char *riscv_isa_string(RISCVCPU *cpu) { int i; -size_t maxlen = 5 + ctz32(cpu->env.misa); +size_t maxlen = 8 + ctpop64(cpu->env.misa); char *isa_string = g_new0(char, maxlen); snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); for (i = 0; i < sizeof(riscv_exts); i++) { -- 2.7.0