Fix CountPopulation non-builtin implementation

The existing non-builtin implementation is returning wrong results.
For example, given the value 63 as a uint8_t it returns 38 (should be 6).

The new implementation follows the naive algorithm presented in figure 5-1
in Hacker's Delight section 5-1.
Note that the algorithm in the book is designed for 32 bit numbers, so we
extended it to support 64 bit as well.

Bug: chromium:1056170
Change-Id: I8fed9c449f80b01b8cc93d339529c0e1e0863fc0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2199345Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67801}

src/base/bits.h

@@ -32,22 +32,27 @@ constexpr inline
   return sizeof(T) == 8 ? __builtin_popcountll(static_cast<uint64_t>(value))
                         : __builtin_popcount(static_cast<uint32_t>(value));
 #else
+  // Fall back to divide-and-conquer popcount (see "Hacker's Delight" by Henry
+  // S. Warren, Jr.), chapter 5-1.
   constexpr uint64_t mask[] = {0x5555555555555555, 0x3333333333333333,
                                0x0f0f0f0f0f0f0f0f};
-  // Start with 1 bit wide buckets of [0,1].
+  // Start with 64 buckets of 1 bits, holding values from [0,1].
   value = ((value >> 1) & mask[0]) + (value & mask[0]);
-  // Having 2 bit wide buckets of [0,2] now.
+  // Having 32 buckets of 2 bits, holding values from [0,2] now.
   value = ((value >> 2) & mask[1]) + (value & mask[1]);
-  // Having 4 bit wide buckets of [0,4] now.
-  value = (value >> 4) + value;
-  // Having 4 bit wide buckets of [0,8] now.
-  if (sizeof(T) > 1)
-    value = ((value >> (sizeof(T) > 1 ? 8 : 0)) & mask[2]) + (value & mask[2]);
-  // Having 8 bit wide buckets of [0,16] now.
+  // Having 16 buckets of 4 bits, holding values from [0,4] now.
+  value = ((value >> 4) & mask[2]) + (value & mask[2]);
+  // Having 8 buckets of 8 bits, holding values from [0,8] now.
+  // From this point on, the buckets are bigger than the number of bits
+  // required to hold the values, and the buckets are bigger the maximum
+  // result, so there's no need to mask value anymore, since there's no
+  // more risk of overflow between buckets.
+  if (sizeof(T) > 1) value = (value >> (sizeof(T) > 1 ? 8 : 0)) + value;
+  // Having 4 buckets of 16 bits, holding values from [0,16] now.
   if (sizeof(T) > 2) value = (value >> (sizeof(T) > 2 ? 16 : 0)) + value;
-  // Having 8 bit wide buckets of [0,32] now.
+  // Having 2 buckets of 32 bits, holding values from [0,32] now.
   if (sizeof(T) > 4) value = (value >> (sizeof(T) > 4 ? 32 : 0)) + value;
-  // Having 8 bit wide buckets of [0,64] now.
+  // Having 1 buckets of 64 bits, holding values from [0,64] now.
   return static_cast<unsigned>(value & 0xff);
 #endif
 }

test/unittests/base/bits-unittest.cc

@@ -18,6 +18,15 @@ namespace v8 {
 namespace base {
 namespace bits {
 
+TEST(Bits, CountPopulation8) {
+  EXPECT_EQ(0u, CountPopulation(uint8_t{0}));
+  EXPECT_EQ(1u, CountPopulation(uint8_t{1}));
+  EXPECT_EQ(2u, CountPopulation(uint8_t{0x11}));
+  EXPECT_EQ(4u, CountPopulation(uint8_t{0x0F}));
+  EXPECT_EQ(6u, CountPopulation(uint8_t{0x3F}));
+  EXPECT_EQ(8u, CountPopulation(uint8_t{0xFF}));
+}
+
 TEST(Bits, CountPopulation16) {
   EXPECT_EQ(0u, CountPopulation(uint16_t{0}));
   EXPECT_EQ(1u, CountPopulation(uint16_t{1}));

test/unittests/heap/cppgc/object-start-bitmap-unittest.cc

@@ -46,7 +46,7 @@ class Object {
   }
 
   // Allow implicitly converting Object to Address.
-  operator Address() const { return address(); }  // NOLINT
+  operator Address() const { return address(); }
 
  private:
   const size_t number_;
@@ -114,12 +114,7 @@ TEST(ObjectStartBitmapTest, AdjacentObjectsAtBegin) {
   EXPECT_EQ(2u, count);
 }
 
-#if defined(V8_CC_MSVC)
-#define MAYBE_AdjacentObjectsAtEnd DISABLED_AdjacentObjectsAtEnd
-#else  // !defined(V8_CC_MSVC)
-#define MAYBE_AdjacentObjectsAtEnd AdjacentObjectsAtEnd
-#endif  // !defined(V8_CC_MSVC)
-TEST(ObjectStartBitmapTest, MAYBE_AdjacentObjectsAtEnd) {
+TEST(ObjectStartBitmapTest, AdjacentObjectsAtEnd) {
   ObjectStartBitmap bitmap(Object::kBaseOffset);
   const size_t last_entry_index = ObjectStartBitmap::MaxEntries() - 1;
   Object object0(last_entry_index - 1);