| Message ID | 1372348062-4516-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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~
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); } [...]
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
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
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(+)