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.
