Re: [Qemu-devel] [RFC] bitops: Provide sext32() and sext64() for signextending bitfields

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> A common operation in instruction decoding is to take a field
> from an instruction that represents a signed integer in some
> arbitrary number of bits, and sign extend it into a C signed
> integer type for manipulation. Provide new functions sext32()
> and sext64() to abstract away the bit manipulation.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I think we've vaguely tossed around the idea of a function to
> abstract away the concept of doing a signextension before,
> so here's an RFC...
>
> Does the API look right? The other approach I thought of would
> be to have functions sextract32()/sextract64() which work like
> the existing extract{32,64} but return signed (and sign
> extended) values, but providing the raw sign-extension separately
> seemed more flexible. (If we want the sextract ops then we could
> implement them as sext32(extract32(value, start, length), length).)

If you have sextractN(), then sextN(v, l) == sextractN(v, 0, l), isn't
it?  I'd expect even a moderately competent optimizer to optimize such
uses of sextractN() just as well as your sextN().

> This implementation continues to rely on the behaviour of right-shift
> of signed integers (as do most of the places which open-code this
> operation today; see also HACKING section 6). If we decide in future
> that we'd rather do this in a strictly-portable way we'll have a
> single place we need to change.
>
> (PS: Not really very tested yet :-))
>
>  include/qemu/bitops.h |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index affcc96..5c6a756 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -273,4 +273,44 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,
>      return (value & ~mask) | ((fieldval << start) & mask);
>  }
>  
> +/**
> + * sext32:
> + * @value: value to sign-extend
> + * @length: length of the bitfield in value
> + *
> + * Sign-extend the least significant @length bits in @value to
> + * a 32 bit signed integer. That is to say, bits [0..@length-2]
> + * are untouched, and bit [@length-1] is duplicated into all
> + * higher bits of the returned value.
> + *
> + * Returns: the sign-extended value of the bitfield.
> + */
> +static inline int32_t sext32(uint32_t value, int length)
> +{
> +    /* Note that this implementation relies on right shift of signed
> +     * integers being an arithmetic shift.
> +     */
> +    return ((int32_t)(value << (32 - length))) >> length;
> +}

Err, shouldn't you shift right by (32 - length), too?

Here's sextract32(), not even compile-tested:

static inline int32_t sextract32(uint32_t value, int start, int length)
{
    return ((int32_t)(value << (32 - length - start))) >> (32 - length);
}

[...]