Addres comments

Author: snehasish

compiler-rt/lib/memprof/tests/histogram_encoding.cpp

@@ -21,7 +21,7 @@ TEST(MemProf, F16EncodeDecode) {
 
     if (TestCase >= MaxRepresentable) {
       EXPECT_EQ(Decoded, MaxRepresentable);
-    } else if (TestCase == 0) {
+    } else if (TestCase <= MaxMantissa) {
       EXPECT_EQ(Decoded, TestCase);
     } else {
       // The decoded value should be close to the original value.

llvm/lib/ProfileData/MemProfReader.cpp

@@ -135,39 +135,7 @@ readMemInfoBlocksV3(const char *Ptr) {
 }
 
 llvm::SmallVector<std::pair<uint64_t, MemInfoBlock>>
-readMemInfoBlocksV4(const char *Ptr) {
-  using namespace support;
-
-  const uint64_t NumItemsToRead =
-      endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Ptr);
-
-  llvm::SmallVector<std::pair<uint64_t, MemInfoBlock>> Items;
-  for (uint64_t I = 0; I < NumItemsToRead; I++) {
-    const uint64_t Id =
-        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Ptr);
-    // We cheat a bit here and remove the const from cast to set the
-    // Histogram Pointer to newly allocated buffer.
-    MemInfoBlock MIB = *reinterpret_cast<const MemInfoBlock *>(Ptr);
-
-    // Only increment by size of MIB since readNext implicitly increments.
-    Ptr += sizeof(MemInfoBlock);
-
-    if (MIB.AccessHistogramSize > 0) {
-      MIB.AccessHistogram =
-          (uintptr_t)malloc(MIB.AccessHistogramSize * sizeof(uint64_t));
-    }
-
-    for (uint64_t J = 0; J < MIB.AccessHistogramSize; J++) {
-      ((uint64_t *)MIB.AccessHistogram)[J] =
-          endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Ptr);
-    }
-    Items.push_back({Id, MIB});
-  }
-  return Items;
-}
-
-llvm::SmallVector<std::pair<uint64_t, MemInfoBlock>>
-readMemInfoBlocksV5(const char *Ptr) {
+readMemInfoBlocksCommon(const char *Ptr, bool IsHistogramEncoded = false) {
   using namespace support;
 
   const uint64_t NumItemsToRead =
@@ -186,19 +154,34 @@ readMemInfoBlocksV5(const char *Ptr) {
       MIB.AccessHistogram =
           (uintptr_t)malloc(MIB.AccessHistogramSize * sizeof(uint64_t));
       for (uint64_t J = 0; J < MIB.AccessHistogramSize; J++) {
-        // The on-disk format for V5 uses uint16_t which is then decoded to
-        // uint64_t.
-        const uint16_t Val =
-            endian::readNext<uint16_t, llvm::endianness::little, unaligned>(
-                Ptr);
-        ((uint64_t *)MIB.AccessHistogram)[J] = decodeHistogramCount(Val);
+        if (!IsHistogramEncoded) {
+          ((uint64_t *)MIB.AccessHistogram)[J] =
+              endian::readNext<uint64_t, llvm::endianness::little, unaligned>(
+                  Ptr);
+        } else {
+          // The encoded on-disk format (V5 onwards) uses uint16_t.
+          const uint16_t Val =
+              endian::readNext<uint16_t, llvm::endianness::little, unaligned>(
+                  Ptr);
+          ((uint64_t *)MIB.AccessHistogram)[J] = decodeHistogramCount(Val);
+        }
       }
     }
     Items.push_back({Id, MIB});
   }
   return Items;
 }
 
+llvm::SmallVector<std::pair<uint64_t, MemInfoBlock>>
+readMemInfoBlocksV4(const char *Ptr) {
+  return readMemInfoBlocksCommon(Ptr);
+}
+
+llvm::SmallVector<std::pair<uint64_t, MemInfoBlock>>
+readMemInfoBlocksV5(const char *Ptr) {
+  return readMemInfoBlocksCommon(Ptr, /*IsHistogramEncoded=*/true);
+}
+
 CallStackMap readStackInfo(const char *Ptr) {
   using namespace support;