qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount t


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH v2] RISC-V: Fix riscv_isa_string, use popcount to count bits
Date: Sun, 11 Mar 2018 12:46:31 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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 <address@hidden>
>> Cc: Peter Maydell <address@hidden>
>> Cc: Eric Blake <address@hidden>
>> Signed-off-by: Michael Clark <address@hidden>
>> ---
>>  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.
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]