| Message ID | 1340653135-26749-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 25 Jun 2012, Peter Maydell wrote: > Add field32() and field64() functions which extract a particular > bit field from a word and return it. Based on an idea by Jia Liu. > > Suggested-by: Jia Liu <proljc@gmail.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- [..snip..] > +static inline uint64_t field64(uint64_t value, int start, int length) > +{ > + assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64); > + return (value >> start) & (~0ULL >> (64 - length)); > +} > + > +/** > + * field32 - return a specified bit field from a uint32_t value > + * @value: The value to extract the bit field from > + * @start: The lowest bit in the bit field (numbered from 0) > + * @length: The length of the bit field > + * > + * Returns the value of the bit field extracted from the input value. > + */ > +static inline uint32_t field32(uint32_t value, int start, int length) > +{ > + assert(start >= 0 && start <= 31 && length > 0 && start + length <= 32); > + return (value >> start) & ~0U >> (32 - length); > +} > + > #endif > The field32 and field64 are inconsistent w.r.t. grouping, while both are correct, one has to go through precedence list mentally in the latter case.
On 25 June 2012 21:26, malc <av1474@comtv.ru> wrote: > The field32 and field64 are inconsistent w.r.t. grouping, while both are > correct, one has to go through precedence list mentally in the latter > case. Yeah, unintentional and I agree that if you have to think about operator precedence there weren't enough brackets. Will fix field32 to match field64 in v2. -- PMM
On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > Add field32() and field64() functions which extract a particular > bit field from a word and return it. Based on an idea by Jia Liu. > > Suggested-by: Jia Liu <proljc@gmail.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Jia Liu had a function like this in the OpenRISC support patchset; > this implementation borrows the API but has a different implementation, > because I wanted to handle the length == wordsize case. > I've also provided a 64 bit version as well as a 32 bit one (alas, gcc > is not smart enough to notice that it only needs to do 32 bit arithmetic > if you pass in a uint32_t to the 64 bit function). > Based on previous experience with a different codebase I think that this > will result in much more comprehensible code than manual shift-and-mask > which is what we tend to do today. > > No users yet, but I wanted to throw this out for review anyway. If > people really don't want it until it gets a first user I can throw it > into my next random patchset that does bit ops... I like this. > > bitops.h | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/bitops.h b/bitops.h > index 07d1a06..36e4c78 100644 > --- a/bitops.h > +++ b/bitops.h > @@ -269,4 +269,32 @@ static inline unsigned long hweight_long(unsigned long w) > return count; > } > > +/** > + * field64 - return a specified bit field from a uint64_t value > + * @value: The value to extract the bit field from > + * @start: The lowest bit in the bit field (numbered from 0) > + * @length: The length of the bit field > + * > + * Returns the value of the bit field extracted from the input value. > + */ > +static inline uint64_t field64(uint64_t value, int start, int length) start and length could be unsigned. > +{ > + assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64); > + return (value >> start) & (~0ULL >> (64 - length)); > +} > + > +/** > + * field32 - return a specified bit field from a uint32_t value > + * @value: The value to extract the bit field from > + * @start: The lowest bit in the bit field (numbered from 0) > + * @length: The length of the bit field > + * > + * Returns the value of the bit field extracted from the input value. > + */ > +static inline uint32_t field32(uint32_t value, int start, int length) > +{ > + assert(start >= 0 && start <= 31 && length > 0 && start + length <= 32); > + return (value >> start) & ~0U >> (32 - length); > +} > + > #endif > -- > 1.7.1 >
On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: > On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> +static inline uint64_t field64(uint64_t value, int start, int length) > > start and length could be unsigned. They could be, but is there any reason why they should be? set_bit(), clear_bit() etc use 'int' for bit numbers, so this is consistent with that. -- PMM
On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> +static inline uint64_t field64(uint64_t value, int start, int length) >> >> start and length could be unsigned. > > They could be, but is there any reason why they should be? > set_bit(), clear_bit() etc use 'int' for bit numbers, so this > is consistent with that. Negative shifts don't work, the line with assert() would get shorter and simpler and I like unsigned values. > > -- PMM
On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: > On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>> >>> start and length could be unsigned. >> >> They could be, but is there any reason why they should be? >> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >> is consistent with that. > > Negative shifts don't work, the line with assert() would get shorter > and simpler and I like unsigned values. I don't like using unsigned for numbers that merely happen to always be positive (as opposed to actually requiring unsigned arithmetic)[*], so I think I'll stick with being consistent with the existing bitops functions, thanks :-) [*] the classic example of where that kind of thing can trip you up is the way it complicates the termination condition on a "for (i = N; i >= 0; i--)" decreasing loop. -- PMM
On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>>> >>>> start and length could be unsigned. >>> >>> They could be, but is there any reason why they should be? >>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >>> is consistent with that. >> >> Negative shifts don't work, the line with assert() would get shorter >> and simpler and I like unsigned values. > > I don't like using unsigned for numbers that merely happen to always > be positive (as opposed to actually requiring unsigned arithmetic)[*], > so I think I'll stick with being consistent with the existing bitops > functions, thanks :-) Using unsigned types also produces better code in some cases. There are also operations which do not work well with signed integers (%, >>). > > [*] the classic example of where that kind of thing can trip you up > is the way it complicates the termination condition on a "for (i = N; > i >= 0; i--)" decreasing loop. > > -- PMM
Blue Swirl <blauwirbel@gmail.com> writes: > On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>>>> >>>>> start and length could be unsigned. >>>> >>>> They could be, but is there any reason why they should be? >>>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >>>> is consistent with that. >>> >>> Negative shifts don't work, the line with assert() would get shorter >>> and simpler and I like unsigned values. >> >> I don't like using unsigned for numbers that merely happen to always >> be positive (as opposed to actually requiring unsigned arithmetic)[*], >> so I think I'll stick with being consistent with the existing bitops >> functions, thanks :-) Consistency is a strong argument. > Using unsigned types also produces better code in some cases. There Better code is an argument only if the effect can be demonstrated. > are also operations which do not work well with signed integers (%, > >>). Operator >> is applicable here. It works exactly as well for negative right operand as it does for large positive right operand: undefined behavior. >> [*] the classic example of where that kind of thing can trip you up >> is the way it complicates the termination condition on a "for (i = N; >> i >= 0; i--)" decreasing loop. Yup. There are more, e.g. fun with unwanted truncation or sign extension when mixing different integer types. Stick to int unless you have a compelling reason.
On Wed, Jun 27, 2012 at 6:01 AM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>>>>> >>>>>> start and length could be unsigned. >>>>> >>>>> They could be, but is there any reason why they should be? >>>>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >>>>> is consistent with that. >>>> >>>> Negative shifts don't work, the line with assert() would get shorter >>>> and simpler and I like unsigned values. >>> >>> I don't like using unsigned for numbers that merely happen to always >>> be positive (as opposed to actually requiring unsigned arithmetic)[*], >>> so I think I'll stick with being consistent with the existing bitops >>> functions, thanks :-) > > Consistency is a strong argument. Yes, but if using unsigned ints for the all bit ops makes sense, they all should be converted. This case could start using unsigned ints before that. It's the same as CODING_STYLE. > >> Using unsigned types also produces better code in some cases. There > > Better code is an argument only if the effect can be demonstrated. I don't know even for which compilers or CPUs this is true so it's unlikely I could demonstrate it. However, googling finds a few articles in defense of this. > >> are also operations which do not work well with signed integers (%, >> >>). > > Operator >> is applicable here. It works exactly as well for negative > right operand as it does for large positive right operand: undefined > behavior. Score one for the unsigned which avoids the negative case. >>> [*] the classic example of where that kind of thing can trip you up >>> is the way it complicates the termination condition on a "for (i = N; >>> i >= 0; i--)" decreasing loop. > > Yup. There are more, e.g. fun with unwanted truncation or sign > extension when mixing different integer types. Yes, mixing signedness is not fun. > Stick to int unless you > have a compelling reason. I've seen exactly opposite guidance: use unsigned whenever possible. In this case, using signed values for bit numbers is never useful. The bit numbers simply are not meaningful if negative, so unsigned values are more intuitive. The types could be even smaller, like uint8_t.
Blue Swirl <blauwirbel@gmail.com> writes: > On Wed, Jun 27, 2012 at 6:01 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@gmail.com> writes: >> >>> On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>>>>>> >>>>>>> start and length could be unsigned. >>>>>> >>>>>> They could be, but is there any reason why they should be? >>>>>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >>>>>> is consistent with that. >>>>> >>>>> Negative shifts don't work, the line with assert() would get shorter >>>>> and simpler and I like unsigned values. >>>> >>>> I don't like using unsigned for numbers that merely happen to always >>>> be positive (as opposed to actually requiring unsigned arithmetic)[*], >>>> so I think I'll stick with being consistent with the existing bitops >>>> functions, thanks :-) >> >> Consistency is a strong argument. > > Yes, but if using unsigned ints for the all bit ops makes sense, they > all should be converted. This case could start using unsigned ints > before that. It's the same as CODING_STYLE. > >> >>> Using unsigned types also produces better code in some cases. There >> >> Better code is an argument only if the effect can be demonstrated. > > I don't know even for which compilers or CPUs this is true so it's > unlikely I could demonstrate it. However, googling finds a few > articles in defense of this. Hearsay. Your honor, I rest my case :) >>> are also operations which do not work well with signed integers (%, >>> >>). >> >> Operator >> is applicable here. It works exactly as well for negative >> right operand as it does for large positive right operand: undefined >> behavior. > > Score one for the unsigned which avoids the negative case. No, my point is: with unsigned the negative case turns into the large positive case, which is exactly as bad. >>>> [*] the classic example of where that kind of thing can trip you up >>>> is the way it complicates the termination condition on a "for (i = N; >>>> i >= 0; i--)" decreasing loop. >> >> Yup. There are more, e.g. fun with unwanted truncation or sign >> extension when mixing different integer types. > > Yes, mixing signedness is not fun. > >> Stick to int unless you >> have a compelling reason. > > I've seen exactly opposite guidance: use unsigned whenever possible. Leads to much dangerous mixing of different integer types, and ugly casts all over the place. > In this case, using signed values for bit numbers is never useful. The > bit numbers simply are not meaningful if negative, so unsigned values > are more intuitive. The types could be even smaller, like uint8_t. Narrow parameter types only hide programming errors here, because arguments get silently truncated before they can reach the protective assertion.
On Thu, Jun 28, 2012 at 5:58 AM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> On Wed, Jun 27, 2012 at 6:01 AM, Markus Armbruster <armbru@redhat.com> wrote: >>> Blue Swirl <blauwirbel@gmail.com> writes: >>> >>>> On Tue, Jun 26, 2012 at 6:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>> On 26 June 2012 19:25, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>> On Tue, Jun 26, 2012 at 6:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>>> On 26 June 2012 18:58, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>>>> On Mon, Jun 25, 2012 at 7:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>>>>>> +static inline uint64_t field64(uint64_t value, int start, int length) >>>>>>>> >>>>>>>> start and length could be unsigned. >>>>>>> >>>>>>> They could be, but is there any reason why they should be? >>>>>>> set_bit(), clear_bit() etc use 'int' for bit numbers, so this >>>>>>> is consistent with that. >>>>>> >>>>>> Negative shifts don't work, the line with assert() would get shorter >>>>>> and simpler and I like unsigned values. >>>>> >>>>> I don't like using unsigned for numbers that merely happen to always >>>>> be positive (as opposed to actually requiring unsigned arithmetic)[*], >>>>> so I think I'll stick with being consistent with the existing bitops >>>>> functions, thanks :-) >>> >>> Consistency is a strong argument. >> >> Yes, but if using unsigned ints for the all bit ops makes sense, they >> all should be converted. This case could start using unsigned ints >> before that. It's the same as CODING_STYLE. >> >>> >>>> Using unsigned types also produces better code in some cases. There >>> >>> Better code is an argument only if the effect can be demonstrated. >> >> I don't know even for which compilers or CPUs this is true so it's >> unlikely I could demonstrate it. However, googling finds a few >> articles in defense of this. > > Hearsay. Your honor, I rest my case :) Objection: the internet can be a veritable source. >>>> are also operations which do not work well with signed integers (%, >>>> >>). >>> >>> Operator >> is applicable here. It works exactly as well for negative >>> right operand as it does for large positive right operand: undefined >>> behavior. >> >> Score one for the unsigned which avoids the negative case. > > No, my point is: with unsigned the negative case turns into the large > positive case, which is exactly as bad. Only if you use signed integers in the first place and then are also careless about conversion. >>>>> [*] the classic example of where that kind of thing can trip you up >>>>> is the way it complicates the termination condition on a "for (i = N; >>>>> i >= 0; i--)" decreasing loop. >>> >>> Yup. There are more, e.g. fun with unwanted truncation or sign >>> extension when mixing different integer types. >> >> Yes, mixing signedness is not fun. >> >>> Stick to int unless you >>> have a compelling reason. >> >> I've seen exactly opposite guidance: use unsigned whenever possible. > > Leads to much dangerous mixing of different integer types, and ugly > casts all over the place. The same argument would also apply to integers of different sizes, use of which can cause dangerous mixing and ugly casts too. But it's not possible to use only signed or unsigned types (and never the other) for a program with size of QEMU. In this case if only one type would be allowed, it would have to be uintptr_t since we absolutely need that but then the set of arithmetic operations allowed could be too limited. >> In this case, using signed values for bit numbers is never useful. The >> bit numbers simply are not meaningful if negative, so unsigned values >> are more intuitive. The types could be even smaller, like uint8_t. > > Narrow parameter types only hide programming errors here, because > arguments get silently truncated before they can reach the protective > assertion. We could enable the compiler warning about truncating and sign changing operations, -Wconversion to find the suspect cases.
On 06/28/2012 08:58 AM, Markus Armbruster wrote: >>> >>> Better code is an argument only if the effect can be demonstrated. >> >> I don't know even for which compilers or CPUs this is true so it's >> unlikely I could demonstrate it. However, googling finds a few >> articles in defense of this. > > Hearsay. Your honor, I rest my case :) On x86_64, conversion from unsigned to unsigned long takes zero instructions, but conversion from int to long takes one instruction. So expressions like a[i] are one instruction shorter if the index is unsigned. unsigned is also slightly safer from a security perspective, since you only need to consider overflow, not underflow. I used to be an int fan but I have been converted. My fingers still prefer int though.
On Sun, Jul 8, 2012 at 11:37 AM, Avi Kivity <avi@redhat.com> wrote: > On 06/28/2012 08:58 AM, Markus Armbruster wrote: >>>> >>>> Better code is an argument only if the effect can be demonstrated. >>> >>> I don't know even for which compilers or CPUs this is true so it's >>> unlikely I could demonstrate it. However, googling finds a few >>> articles in defense of this. >> >> Hearsay. Your honor, I rest my case :) > > On x86_64, conversion from unsigned to unsigned long takes zero > instructions, but conversion from int to long takes one instruction. So > expressions like a[i] are one instruction shorter if the index is unsigned. > > unsigned is also slightly safer from a security perspective, since you > only need to consider overflow, not underflow. Q.E.D. Expect patches to convert bitops to unsigned shortly. > > I used to be an int fan but I have been converted. My fingers still > prefer int though. > > -- > error compiling committee.c: too many arguments to function > >
diff --git a/bitops.h b/bitops.h index 07d1a06..36e4c78 100644 --- a/bitops.h +++ b/bitops.h @@ -269,4 +269,32 @@ static inline unsigned long hweight_long(unsigned long w) return count; } +/** + * field64 - return a specified bit field from a uint64_t value + * @value: The value to extract the bit field from + * @start: The lowest bit in the bit field (numbered from 0) + * @length: The length of the bit field + * + * Returns the value of the bit field extracted from the input value. + */ +static inline uint64_t field64(uint64_t value, int start, int length) +{ + assert(start >= 0 && start <= 63 && length > 0 && start + length <= 64); + return (value >> start) & (~0ULL >> (64 - length)); +} + +/** + * field32 - return a specified bit field from a uint32_t value + * @value: The value to extract the bit field from + * @start: The lowest bit in the bit field (numbered from 0) + * @length: The length of the bit field + * + * Returns the value of the bit field extracted from the input value. + */ +static inline uint32_t field32(uint32_t value, int start, int length) +{ + assert(start >= 0 && start <= 31 && length > 0 && start + length <= 32); + return (value >> start) & ~0U >> (32 - length); +} + #endif
Add field32() and field64() functions which extract a particular bit field from a word and return it. Based on an idea by Jia Liu. Suggested-by: Jia Liu <proljc@gmail.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Jia Liu had a function like this in the OpenRISC support patchset; this implementation borrows the API but has a different implementation, because I wanted to handle the length == wordsize case. I've also provided a 64 bit version as well as a 32 bit one (alas, gcc is not smart enough to notice that it only needs to do 32 bit arithmetic if you pass in a uint32_t to the 64 bit function). Based on previous experience with a different codebase I think that this will result in much more comprehensible code than manual shift-and-mask which is what we tend to do today. No users yet, but I wanted to throw this out for review anyway. If people really don't want it until it gets a first user I can throw it into my next random patchset that does bit ops... bitops.h | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)