Re: [PATCH v1 6/8] s390x/tcg: Implement MULTIPLY SINGLE (MSC, MSGC, MSGRKC, MSRKC)

[David Hildenbrand]

On 26.09.20 00:06, Richard Henderson wrote:
> On 9/22/20 3:31 AM, David Hildenbrand wrote:
>> +static uint32_t cc_calc_muls_32(int64_t res)
>> +{
>> +    /* Arithmetic shift with sign extension so we can compare against 
>> -1ull. */
>> +    const uint64_t tmp = res >> 31;
>> +
>> +    if (!res) {
>> +        return 0;
>> +    } else if (!(!tmp || tmp == -1ull)) {
> 
> Comparing signed vs unsigned.  Use -1 without suffix.

tmp is also uint64_t - but I can change that to int64_t.

(and condense to "tmp && tmp != -1")

> 
>> +static uint64_t cc_calc_muls_64(int64_t res_high, uint64_t res_low)
>> +{
>> +    const uint8_t tmp = res_low >> 63;
>> +
>> +    if (!res_high && !res_low) {
>> +        return 0;
>> +    } else if (!(!res_high && !tmp) || !(res_high == -1ull && tmp)) {
> 
> This simplifies to res_high + tmp != 0.

Yeah, after messing up one time I decided to phrase it just as stated in
the PoP.

> 
> Probably better to keep tmp as uint64_t; otherwise we're likely to have an
> unnecessary zero-extension from uint8_t to uint64_t.
> Or, drop 'tmp' altogether and use
> 
>   if (res_high + (res_low >> 63) != 0)

Thanks, I'll go with that.

> 
> Otherwise, looks good.
> 
> 
> r~
> 


-- 
Thanks,

David / dhildenb