RFR: 8234331: Add robust and optimized utility for rounding up to next power of two+

[Thomas Stüfe]

Hi Claes,

this looks nice, thank you for your work. Only minor stuff remains, mostly
matters of taste. Feel free to ignore them. The patch is already fine as it
is for me.

With your latest patch, AIX still works.

---

http://cr.openjdk.java.net/~redestad/8234331/open.04/src/hotspot/share/utilities/count_leading_zeros.hpp.udiff.html

Taste matter: I find the return type for count_leading_zeros as uint32_t
oddly specific. Instinctively I would have choosen int or unsigned. The gcc
buildins all return int.

---

Question, why do we treat signed numbers of 8 and 16 bits different wrt to
values < 0?

if you leave behavior as it is, could you please adjust the comment:

 // Return the number of leading zeros in x, e.g. the zero-based index
 // of the most significant set bit in x.  Undefined for 0.
+ // For signed numbers whose size is < 32bit, function will
+ // return 0 for values < 0.

----

Taste matter: Windows, 64bit version for x86: You could call your own
count_leading_zeros<uint32_t>, instead of calling _BitScanReverse.

----

http://cr.openjdk.java.net/~redestad/8234331/open.04/test/hotspot/gtest/utilities/test_powerOfTwo.cpp.html

In the test for next_up, can we have, for completeness sake, a test which
tests that with any valid input pow2 the result is the next pow2?

--- a/test/hotspot/gtest/utilities/test_powerOfTwo.cpp  Thu Dec 05 02:28:58
2019 +0100
+++ b/test/hotspot/gtest/utilities/test_powerOfTwo.cpp  Thu Dec 05 11:08:47
2019 +0100
@@ -139,6 +139,11 @@
     EXPECT_EQ(pow2, next_power_of_2(pow2 - 1))
       << "value = " << pow2 - 1;
   }
+  // next(pow2) should return pow2 * 2
+  for (T pow2 = T(1); pow2 < t_max_pow2 / 2; pow2 = pow2 * 2) {
+    EXPECT_EQ(pow2 * 2, next_power_of_2(pow2))
+      << "value = " << pow2;
+  }

---

Thanks, Thomas

On Thu, Dec 5, 2019 at 2:32 AM Claes Redestad <claes.redestad at oracle.com>
wrote:

> Hi again,
>
> On 2019-12-04 18:37, Claes Redestad wrote:
> >>
> >>
> >>
> http://cr.openjdk.java.net/~redestad/8234331/open.03/src/hotspot/share/utilities/count_leading_zeros.hpp.frames.html
> >>
> >>
> >> template <typename T> inline uint32_t count_leading_zeros(T x) {
> >>
> >> Instead of all the "if sizeof(T) == 4|8", could we specialize the
> >> functions for different operand sizes? Example:
> >>
> >> template <typename T, size_t n> struct Impl;
> >> template <typename T> struct Impl<T, 8> { static int doit(T v) {
> >> return __builtin_clzll(v); } };
> >> template <typename T> struct Impl<T, 4> { static int doit(T v) {
> >> return __builtin_clz(v); } };
> >> template <typename T> int count_leading_zero(T v) { return Impl<T,
> >> sizeof(T)>::doit(v); }
> >>
> >> Would make it easier later to plug in implementations for different
> >> sizes, and we'd still get a compile time error for invalid input types.
> >
> > I guess we could, but I'm not so sure it'd help readability.
>
> after some considerations, I took this idea for a spin, along with
> implementations and tests for 8 and 16-bit for completeness up
> front:
>
> http://cr.openjdk.java.net/~redestad/8234331/open.04
>
> I also turned tests into template functions etc. I think it turned
> out an improvement.
>
> Testing: tier1-3 (some ongoing)
>
> Thanks!
>
> /Claes
>