On 04/11/2015 12: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.
That does the job, but it also does look like a typo... Paolo
