Re: [PATCH v2] target/ppc: Fix 64-bit decrementer

[Cédric Le Goater]

On 9/14/21 3:21 PM, Richard Henderson wrote:
> On 9/14/21 2:47 AM, Cédric Le Goater wrote:
>> On 9/14/21 11:19 AM, Peter Maydell wrote:
>>>>       /* Truncate value to decr_width and sign extend for simplicity */
>>>> -    value &= ((1ULL << nr_bits) - 1);
>>>> +    value = sextract64(value, 0, nr_bits);
>>>>       negative = !!(value & (1ULL << (nr_bits - 1)));
>>>>       if (negative) {
>>>>           value |= (0xFFFFFFFFULL << nr_bits);
>>>
>>> I think these lines that say "if negative then force all the
>>> high bits to one" are also no longer required. That is, this
>>> entire section of code:
>>>      value &= ((1ULL << nr_bits) - 1);
>>>      negative = !!(value & (1ULL << (nr_bits - 1)));
>>>      if (negative) {
>>>          value |= (0xFFFFFFFFULL << nr_bits);
>>>      }
>>>
>>> is an open-coded sign-extension, which can all be replaced with
>>> the single line
>>>      value = sextract64(value, 0, nr_bits);
>>
>> 'negative' is used for more tests afterwards but you are right. I will respin
>> with more changes.
> 
> After the sign-extension, negative needs no complicated expression.
> 
>   value = sextract64(value, 0, nr_bits);
>   negative = (target_long)value < 0;

Yes. I was wondering about the 'target_ulong' type used for the value 
and decr variables. The code has below : 

    ...
    if ((value < 3) ||
    ...

and then another sign calculation on a target_ulong

       ...
       && !(decr & (1ULL << (nr_bits - 1))))) {
       ...
 
We should introduce intermediate 'int64_t' variables to extract the 
sign values from the target_ulong. That would be cleaner.

Thanks,

C.