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

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

[Peter Maydell]

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).)

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;
+}
+
+/**
+ * sext64:
+ * @value: value to sign-extend
+ * @length: length of the bitfield in value
+ *
+ * Sign-extend the least significant @length bits in @value to
+ * a 64 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 int64_t sext64(uint64_t value, int length)
+{
+    /* Note that this implementation relies on right shift of signed
+     * integers being an arithmetic shift.
+     */
+    return ((int64_t)(value << (64 - length))) >> length;
+}
+
 #endif
-- 
1.7.9.5

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

[Richard Henderson]

On 06/27/2013 08:47 AM, Peter Maydell wrote:
> 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).)

Seems sensible.

I've been wondering if we should provide tcg-op.h helpers for the
same thing -- even without introducing new tcg opcodes yet -- just
for clarity in the translators.

Though of course, the hosts that tend to provide deposit opcodes
also tend to provide extract opcodes...

> 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.

Fair enough.  The patch itself looks good.


r~

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

[Markus Armbruster]

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);
}

[...]

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

[Peter Maydell]

On 27 June 2013 18:58, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> 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().

Good point. I think that biases in favour of just implementing
sextractN().

>> +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?

Doh. I said I hadn't tested this ;-)

> 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);
> }

Looks plausible. We should probably add in the assert() from
extract32().

thanks
-- PMM