On 05/11/2015 00:36, Mark Cave-Ayland wrote:
> On 04/11/15 11:05, Richard Henderson wrote:
>
>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>> int32_t src = rs2 >> (word * 32);
>>>>>>>>> - int64_t scaled = src << scale;
>>>>>>>>> + int64_t scaled = (int64_t)src << scale;
>>>>>>>>> int64_t from_fixed = scaled >> 16;
>> ...
>>>>
>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>> Note that fpackfix requires the same correction. And it wouldn't hurt
>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>> gods.
>>>
>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>
>>> scaled = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>> from_fixed = 0xffffffff00000000 >> 16 = 0x0000ffffffff0000
>>>
>>> Now from_fixed is positive and you get 32767 instead of -32768. In
>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>> and back to int64_t on the from_fixed assignment. I must be
>>> misunderstanding your suggestion.
>>
>> int64_t scaled = (uint64_t)src << scale;
>>
>> I.e. one explicit conversion and one implicit conversion.
>
> I suspect Richard knows more about this part of SPARC emulation than I
> do, so I'd be fine with a solution similar to the above if everyone
> agress. Let me know if you need me to send a SPARC pull request,
> although it will probably be quicker coming from Paolo/Richard at the
> moment.
All solutions work. You have to tell us which you prefer among
/* Has undefined behavior (though no compiler uses it) */
int64_t scaled = (int64_t)src << scale;
/* Seems like a typo */
int64_t scaled = (uint64_t)src << scale;
/* Ugly code */
int64_t scaled = (uint64_t)(int64_t)src << scale;
Paolo
