Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 1/6] target-ppc: Implement unsigned quadword left/right shift and unit tests

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2816 bytes --]

On 01/05/2017 03:45 PM, joserz@linux.vnet.ibm.com wrote:

>>
>>> +    { 0, 0, 0ULL, 0, 200, false },
>>
>> If you are going to support shifts larger than 127, your testsuite
>> should include a shift of a non-zero number.  Also, if you are going to
>> implicitly truncate the shift value into range, then accepting a signed
>> shift might be nicer (as there are cases where it is easier to code a
>> shift by -1 than it is a shift by 127).
> 
> Correct me if I'm wrong: Actually the caller can use the function like:
> 
> urshift(low, high, (uint32_t)-1);
> 
> And I'll get "shift == UINT_MAX", which truncates to 127 after "&= 127".
> I don't need to use a signed integer necessarily, right?

It's not necessary to make the number signed, and in fact, thanks to C
implicit promotion rules and qemu's guarantee that we only compile on
hardware with twos-complement signed numbers, urshift(low, high, -1)
works the same whether the shift parameter is typed as signed or
unsigned (without needing an explicit cast).

It's just that making it signed instead of unsigned makes it slightly
easier to read code by interpreting shifting by -1 as "shift all but the
last bit out" (shifting by -2 is "shift all but the last two bits out",
etc.), where there is no ambiguity what was meant on shift values down
to -127.  In contrast, shifting by UINT_MAX is ambiguous to read without
further consulting the documentation (is it "shift modulo bit width" or
"shift to infinity so that I'm left with 0"?).  Or put another way,
making the shift amount signed makes it OBVIOUS that you are implicitly
masking the shift amount to the maximum width shift (making it perhaps a
bit easier to see that shifting by 128 is a no-op, rather than a synonym
for making the result 0), while leaving the shift amount unsigned almost
argues that you want to assert that the shift amount is in range.


>>> +{
>>> +    shift &= 127;
>>
>> This says you allow any shift value (whether negative or beyond 127);
>> either the testsuite must cover this, or you should tighten the contract
>> and assert that the callers pass a value in range.
> 
> I prefer to let this function flexible and let the caller decides
> whether to assert it or not before calling these functions. But I'm
> totally open to assert it if you prefer.
> 
> I'm writing the testcase to cover it.

At any rate, I'm fine with your decision to state that it is up to the
caller to range-check if truncation was not intended, and that the
worker function implicitly truncates rather than noisily asserting a
range - as long as the v5 patch also includes the documentation and
testsuite coverage of this behavior.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]