Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On Wed, 2019-11-27 at 19:06 +, Robin Murphy wrote: > On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: > > On Wed, 2019-11-27 at 18:06 +, Robin Murphy wrote: > > > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > > > Some users need to make sure their rounding function accepts and > > > > > returns > > > > > 64bit long variables regardless of the architecture. Sadly > > > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > > > new generic 64bit variant of the function and cleanup rougue custom > > > > > implementations. > > > > > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > > > work correctly for any type of variables, instead of creating special > > > > variant for every type? > > > > > > In fact, that is sort of the case already - roundup_pow_of_two() itself > > > wraps ilog2() such that the constant case *is* type-independent. And > > > since ilog2() handles non-constant values anyway, might it be reasonable > > > to just take the strongly-typed __roundup_pow_of_two() helper out of the > > > loop as below? > > > > > > Robin > > > > > > > That looks way better that's for sure. Some questions. > > > > > ->8- > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > > > --- a/include/linux/log2.h > > > +++ b/include/linux/log2.h > > > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > */ > > >#define roundup_pow_of_two(n) \ > > >( \ > > > - __builtin_constant_p(n) ? ( \ > > > - (n == 1) ? 1 : \ > > > - (1UL << (ilog2((n) - 1) + 1)) \ > > > -) : \ > > > - __roundup_pow_of_two(n) \ > > > + (__builtin_constant_p(n) && (n == 1)) ? \ > > > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ > > > > Then here you'd have to use ULL instead of UL, right? I want my 64bit value > > everywhere regardless of the CPU arch. The downside is that would affect > > performance to some extent (i.e. returning a 64bit value where you used to > > have > > a 32bit one)? > > True, although it's possible that 1ULL might result in the same codegen > if the compiler can see that the result is immediately truncated back to > long anyway. Or at worst, I suppose "(typeof(n))1" could suffice, > however ugly. Either way, this diff was only an illustration rather than > a concrete proposal, but it might be an interesting diversion to > investigate. > > On that note, though, you should probably be using ULL in your current > patch too. I actually meant to, the fix got lost. Thanks for pointing it out. As I see Leon also likes this, I'll try out this implementation and come back with some results. Regards, Nicolas signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On Wed, Nov 27, 2019 at 07:06:12PM +, Robin Murphy wrote: > On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: > > On Wed, 2019-11-27 at 18:06 +, Robin Murphy wrote: > > > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > > > Some users need to make sure their rounding function accepts and > > > > > returns > > > > > 64bit long variables regardless of the architecture. Sadly > > > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > > > new generic 64bit variant of the function and cleanup rougue custom > > > > > implementations. > > > > > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > > > work correctly for any type of variables, instead of creating special > > > > variant for every type? > > > > > > In fact, that is sort of the case already - roundup_pow_of_two() itself > > > wraps ilog2() such that the constant case *is* type-independent. And > > > since ilog2() handles non-constant values anyway, might it be reasonable > > > to just take the strongly-typed __roundup_pow_of_two() helper out of the > > > loop as below? > > > > > > Robin > > > > > > > That looks way better that's for sure. Some questions. > > > > > ->8- > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > > > --- a/include/linux/log2.h > > > +++ b/include/linux/log2.h > > > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > */ > > >#define roundup_pow_of_two(n) \ > > >( \ > > > - __builtin_constant_p(n) ? ( \ > > > - (n == 1) ? 1 : \ > > > - (1UL << (ilog2((n) - 1) + 1)) \ > > > -) : \ > > > - __roundup_pow_of_two(n) \ > > > + (__builtin_constant_p(n) && (n == 1)) ? \ > > > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ > > > > Then here you'd have to use ULL instead of UL, right? I want my 64bit value > > everywhere regardless of the CPU arch. The downside is that would affect > > performance to some extent (i.e. returning a 64bit value where you used to > > have > > a 32bit one)? > > True, although it's possible that 1ULL might result in the same codegen if > the compiler can see that the result is immediately truncated back to long > anyway. Or at worst, I suppose "(typeof(n))1" could suffice, however ugly. > Either way, this diff was only an illustration rather than a concrete > proposal, but it might be an interesting diversion to investigate. > > On that note, though, you should probably be using ULL in your current patch > too. > > > Also, what about callers to this function on platforms with 32bit 'unsigned > > longs' that happen to input a 64bit value into this. IIUC we'd have a > > change of > > behaviour. > > Indeed, although the change in such a case would be "start getting the > expected value instead of nonsense", so it might very well be welcome ;) Agree, if code overflowed with 32 bits before this change, the code was already broken. At least now, it won't overflow. > > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On 27/11/2019 6:24 pm, Nicolas Saenz Julienne wrote: On Wed, 2019-11-27 at 18:06 +, Robin Murphy wrote: On 26/11/2019 12:51 pm, Leon Romanovsky wrote: On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: Some users need to make sure their rounding function accepts and returns 64bit long variables regardless of the architecture. Sadly roundup/rounddown_pow_two() takes and returns unsigned longs. Create a new generic 64bit variant of the function and cleanup rougue custom implementations. Is it possible to create general roundup/rounddown_pow_two() which will work correctly for any type of variables, instead of creating special variant for every type? In fact, that is sort of the case already - roundup_pow_of_two() itself wraps ilog2() such that the constant case *is* type-independent. And since ilog2() handles non-constant values anyway, might it be reasonable to just take the strongly-typed __roundup_pow_of_two() helper out of the loop as below? Robin That looks way better that's for sure. Some questions. ->8- diff --git a/include/linux/log2.h b/include/linux/log2.h index 83a4a3ca3e8a..e825f8a6e8b5 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) */ #define roundup_pow_of_two(n)\ (\ - __builtin_constant_p(n) ? ( \ - (n == 1) ? 1 : \ - (1UL << (ilog2((n) - 1) + 1)) \ - ) : \ - __roundup_pow_of_two(n) \ + (__builtin_constant_p(n) && (n == 1)) ? \ + 1 : (1UL << (ilog2((n) - 1) + 1)) \ Then here you'd have to use ULL instead of UL, right? I want my 64bit value everywhere regardless of the CPU arch. The downside is that would affect performance to some extent (i.e. returning a 64bit value where you used to have a 32bit one)? True, although it's possible that 1ULL might result in the same codegen if the compiler can see that the result is immediately truncated back to long anyway. Or at worst, I suppose "(typeof(n))1" could suffice, however ugly. Either way, this diff was only an illustration rather than a concrete proposal, but it might be an interesting diversion to investigate. On that note, though, you should probably be using ULL in your current patch too. Also, what about callers to this function on platforms with 32bit 'unsigned longs' that happen to input a 64bit value into this. IIUC we'd have a change of behaviour. Indeed, although the change in such a case would be "start getting the expected value instead of nonsense", so it might very well be welcome ;) Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On Wed, 2019-11-27 at 18:06 +, Robin Murphy wrote: > On 26/11/2019 12:51 pm, Leon Romanovsky wrote: > > On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > > > Some users need to make sure their rounding function accepts and returns > > > 64bit long variables regardless of the architecture. Sadly > > > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > > > new generic 64bit variant of the function and cleanup rougue custom > > > implementations. > > > > Is it possible to create general roundup/rounddown_pow_two() which will > > work correctly for any type of variables, instead of creating special > > variant for every type? > > In fact, that is sort of the case already - roundup_pow_of_two() itself > wraps ilog2() such that the constant case *is* type-independent. And > since ilog2() handles non-constant values anyway, might it be reasonable > to just take the strongly-typed __roundup_pow_of_two() helper out of the > loop as below? > > Robin > That looks way better that's for sure. Some questions. > ->8- > diff --git a/include/linux/log2.h b/include/linux/log2.h > index 83a4a3ca3e8a..e825f8a6e8b5 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >*/ > #define roundup_pow_of_two(n) \ > ( \ > - __builtin_constant_p(n) ? ( \ > - (n == 1) ? 1 : \ > - (1UL << (ilog2((n) - 1) + 1)) \ > -) : \ > - __roundup_pow_of_two(n) \ > + (__builtin_constant_p(n) && (n == 1)) ? \ > + 1 : (1UL << (ilog2((n) - 1) + 1)) \ Then here you'd have to use ULL instead of UL, right? I want my 64bit value everywhere regardless of the CPU arch. The downside is that would affect performance to some extent (i.e. returning a 64bit value where you used to have a 32bit one)? Also, what about callers to this function on platforms with 32bit 'unsigned longs' that happen to input a 64bit value into this. IIUC we'd have a change of behaviour. Regards, Nicolas signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On 26/11/2019 12:51 pm, Leon Romanovsky wrote: On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: Some users need to make sure their rounding function accepts and returns 64bit long variables regardless of the architecture. Sadly roundup/rounddown_pow_two() takes and returns unsigned longs. Create a new generic 64bit variant of the function and cleanup rougue custom implementations. Is it possible to create general roundup/rounddown_pow_two() which will work correctly for any type of variables, instead of creating special variant for every type? In fact, that is sort of the case already - roundup_pow_of_two() itself wraps ilog2() such that the constant case *is* type-independent. And since ilog2() handles non-constant values anyway, might it be reasonable to just take the strongly-typed __roundup_pow_of_two() helper out of the loop as below? Robin ->8- diff --git a/include/linux/log2.h b/include/linux/log2.h index 83a4a3ca3e8a..e825f8a6e8b5 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -172,11 +172,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n) */ #define roundup_pow_of_two(n) \ ( \ - __builtin_constant_p(n) ? ( \ - (n == 1) ? 1 : \ - (1UL << (ilog2((n) - 1) + 1)) \ - ) : \ - __roundup_pow_of_two(n) \ + (__builtin_constant_p(n) && (n == 1)) ? \ + 1 : (1UL << (ilog2((n) - 1) + 1)) \ ) /** ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On Tue, 2019-11-26 at 10:19 +0100, Nicolas Saenz Julienne wrote: > Some users need to make sure their rounding function accepts and returns > 64bit long variables regardless of the architecture. Sadly > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > new generic 64bit variant of the function and cleanup rougue custom > implementations. > > Signed-off-by: Nicolas Saenz Julienne Small Nit: I corrected the patch subject for next version. linux/log2.h: Add roundup/rounddown_pow_two_u64() family of functions Note the change here: Regards, Nicolas signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/7] linux/log2.h: Add roundup/rounddown_pow_two64() family of functions
On Tue, Nov 26, 2019 at 10:19:39AM +0100, Nicolas Saenz Julienne wrote: > Some users need to make sure their rounding function accepts and returns > 64bit long variables regardless of the architecture. Sadly > roundup/rounddown_pow_two() takes and returns unsigned longs. Create a > new generic 64bit variant of the function and cleanup rougue custom > implementations. Is it possible to create general roundup/rounddown_pow_two() which will work correctly for any type of variables, instead of creating special variant for every type? Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
