Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib: add 64-bit versions of clz, __ffs and log2 ......................................................................
lib: add 64-bit versions of clz, __ffs and log2
Tack on `_64` to the end of the 32-bit name.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Iefc6e6c51f5b20607c88e38660a499a4f77ce0d0 --- M src/include/lib.h 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/45938/1
diff --git a/src/include/lib.h b/src/include/lib.h index 46d8b02..4342b9e 100644 --- a/src/include/lib.h +++ b/src/include/lib.h @@ -56,4 +56,8 @@ /* Integer binary logarithm (rounding up): log2_ceil(0) == -1, log2(5) == 3 */ static inline int log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); }
+static inline long long clz_64(u64 x) { return x ? __builtin_clz(x) : sizeof(x) * 8; } +static inline int log2_64(u64 x) { return sizeof(x) * 8 - clz_64(x) - 1; } +static inline int __ffs_64(u64 x) { return log2_64(x & (u64)(-(s64)x)); } + #endif /* __LIB_H__ */
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45938/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45938/1//COMMIT_MSG@7 PS1, Line 7: lib Could you add these to libpayload as well so the two stay in sync?
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@59 PS1, Line 59: long long Why does one of these return `long long` and the others int? I think int makes sense for all of them.
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@59 PS1, Line 59: __builtin_clz Careful, this is in and off itself a 32-bit function. The only available options for this are __builtin_clzl() and __builtin_clzll()... I guess even on 64-bit systems `unsigned long long` is still 64 bit, so it's probably fine to use __builtin_clzll() everywhere?
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@61 PS1, Line 61: __ffs_64 nit: Linux calls this __ffs64(). Maybe better to use clz64() and __ffs64() for consistency? (Although log2() doesn't match then, Linux doesn't have that one...)
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45938
to look at the new patch set (#2).
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
lib and libpayload: add 64-bit versions of clz, __ffs and log2
Add 64-bit versions of clz, __ffs & log2: `__ffs64`, `__clz64`, and `log2_64`.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Iefc6e6c51f5b20607c88e38660a499a4f77ce0d0 --- M payloads/libpayload/include/libpayload.h M src/include/lib.h 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/45938/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45938/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45938/1//COMMIT_MSG@7 PS1, Line 7: lib
Could you add these to libpayload as well so the two stay in sync?
Done
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@59 PS1, Line 59: __builtin_clz
Careful, this is in and off itself a 32-bit function. […]
I guess I'd rather leave clz alone and just change this one.
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@59 PS1, Line 59: long long
Why does one of these return `long long` and the others int? I think int makes sense for all of them […]
Ack
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@61 PS1, Line 61: __ffs_64
nit: Linux calls this __ffs64(). […]
sure, that works, I'll just change those two and leave log2_64 for now.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/1/src/include/lib.h@59 PS1, Line 59: __builtin_clz
I guess I'd rather leave clz alone and just change this one.
Sorry, by "everywhere" I meant "on every architecture" (even though it's technically architecture-dependent whether int64_t is `long` or `long long`, but I think the latter should still work for both). Of course I was only talking about this function, the 32-bit clz() should still use the 32-bit builtin.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h@59 PS2, Line 59: clz64 nit: IMHO, `clzll` or `clz_ll` feels more consistent, and using `log2_ll` leads to less confusion than `log2_64`.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h@59 PS2, Line 59: clz64
nit: IMHO, `clzll` or `clz_ll` feels more consistent, and using `log2_ll` leads to less confusion th […]
Really? I would say the opposite. I think the whole short, long, long long, etc. C type system is terrible and it's best to avoid using it as much as possible (like we mostly do in coreboot). It leads to exactly these kinds of issues where nobody knows whether a 64-bit value should be long or long long or what, because it's completely dependent on architecture and a bunch of other factors.
Just like the htonl() byte swapping names are terrible and we replace them with clearer stuff like htobe32() where possible, I think we should use exact numbers here as well.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/45938/2/src/include/lib.h@59 PS2, Line 59: clz64
Really? I would say the opposite. I think the whole short, long, long long, etc. […]
Hrm, thinking again, I agree with you... Let me take a look...
Gazing into the realm of introspection reveals that the SNR for this signal is atrociously low. How unsurprising.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45938 )
Change subject: lib and libpayload: add 64-bit versions of clz, __ffs and log2 ......................................................................
lib and libpayload: add 64-bit versions of clz, __ffs and log2
Add 64-bit versions of clz, __ffs & log2: `__ffs64`, `__clz64`, and `log2_64`.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Iefc6e6c51f5b20607c88e38660a499a4f77ce0d0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45938 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/include/libpayload.h M src/include/lib.h 2 files changed, 12 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 01d71b8..475fe0f 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -454,6 +454,14 @@ static inline int log2(u32 x) { return (int)sizeof(x) * 8 - clz(x) - 1; } /* Find First Set: __ffs(0xf) == 0, __ffs(0) == -1, __ffs(1 << 31) == 31 */ static inline int __ffs(u32 x) { return log2(x & (u32)(-(s32)x)); } + +static inline int clz64(u64 x) +{ + return x ? __builtin_clzll(x) : sizeof(x) * 8; +} + +static inline int log2_64(u64 x) { return sizeof(x) * 8 - clz64(x) - 1; } +static inline int __ffs64(u64 x) { return log2_64(x & (u64)(-(s64)x)); } /** @} */
/** diff --git a/src/include/lib.h b/src/include/lib.h index 46d8b02..a0003d3 100644 --- a/src/include/lib.h +++ b/src/include/lib.h @@ -56,4 +56,8 @@ /* Integer binary logarithm (rounding up): log2_ceil(0) == -1, log2(5) == 3 */ static inline int log2_ceil(u32 x) { return (x == 0) ? -1 : log2(x * 2 - 1); }
+static inline int clz64(u64 x) { return x ? __builtin_clzll(x) : sizeof(x) * 8; } +static inline int log2_64(u64 x) { return sizeof(x) * 8 - clz64(x) - 1; } +static inline int __ffs64(u64 x) { return log2_64(x & (u64)(-(s64)x)); } + #endif /* __LIB_H__ */
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Tim Wawrzynczak (Code Review)