[Support] Change TrackingStatistic and NoopStatistic to use uint64_t instead of unsigned

[mingmingl]

Summary

Passes could store large integers into stats, as a result of using block frequency counters. Using uint64_t gives a much smaller chance of overflow and reduces confusion. One example pass is block-placement-stats.

---

About binary size

For bin/clang, https://www.diffchecker.com/xZIbpmYz is a diff of bloaty bin/clang

the FILE SIZE and VM SIZE of bloaty (measured at MiB) doesn't change before and after this change; the .data section increases from 139Ki to 173Ki. Full bloaty output in [1]

[1] https://gist.github.com/minglotus-6/a9af947ab9d6ed120d87682b214c21ac is bloaty bin/clang with unsigned, and https://gist.github.com/minglotus-6/a9af947ab9d6ed120d87682b214c21ac is bloaty bin/clang with uint64_t, measured at the same base commit number, with and without this patch.

[MaskRay]

I think this makes sense. Any chance to compare clang sizes with and without this change? I think it is fine to increase the size a bit as fixing the overflow problem is more important, but the number should still be useful.

llvm/unittests/ADT/StatisticTest.cpp

46

UINT64_C(0) may be better.

[MaskRay]

[Stats] is an abbreviation of an uncommon tag [Statistics] . I'd use [Support] Change TrackingStatistic to use uint64_t instead of unsigned

We check 64-bit atomic support (CMake check_working_cxx_atomics64), so the new use is fine. It may be first time LLVMSupport uses it, though (not checked very carefully).

I'll suggest that postpone this to at least Monday afternoon (skipping weekend) to give other folks a chance to look.

[mingmingl]

In struct Statistic from [1], change field value type from unsigned to uint64_t accordingly.

[1] llvm-project/mlir/lib/Pass/PassStatistics.cpp https://github.com/llvm/llvm-project/blob/86d5d34c72233f75165bc328d8c45979979d126a/mlir/lib/Pass/PassStatistics.cpp#L24

[mingmingl]

In D128070#3592531, @MaskRay wrote:

I think this makes sense. Any chance to compare clang sizes with and without this change? I think it is fine to increase the size a bit as fixing the overflow problem is more important, but the number should still be useful.

Sure; it makes sense to me to measure binary size.

For bin/clang, https://www.diffchecker.com/xZIbpmYz is a diff of bloaty bin/clang

• the FILE SIZE and VM SIZE of bloaty (measured at MiB) doesn't change before and after this change; the .data section increases from 139Ki to 173Ki. Full bloaty output in [1]

[1] https://gist.github.com/minglotus-6/a9af947ab9d6ed120d87682b214c21ac is bloaty bin/clang with unsigned, and https://gist.github.com/minglotus-6/a9af947ab9d6ed120d87682b214c21ac is bloaty bin/clang with uint64_t, measured at the same base commit number, with and without this patch.

In D128070#3593045, @MaskRay wrote:

[Stats] is an abbreviation of an uncommon tag [Statistics] . I'd use [Support] Change TrackingStatistic to use uint64_t instead of unsigned

We check 64-bit atomic support (CMake check_working_cxx_atomics64), so the new use is fine. It may be first time LLVMSupport uses it, though (not checked very carefully).

Thanks for checking check_working_cxx_atomics64.

I'll suggest that postpone this to at least Monday afternoon (skipping weekend) to give other folks a chance to look.

This is also what i'm planning to do. Will hold this patch over the (long) weekend and revisit it on Tuesday.

[mingmingl]

In llvm/unittests/ADT/StatisticTest.cpp line 45, use UINT64_C(0) rather than 0ll as suggested.

[mingmingl]

No change to patch content; 2 updates commit message (keep commit message and differential summary consistent, and append differential revision link as the last line of commit message).

[MaskRay]

You may click Edit Revision to edit the summary and then run arc amend to update local commit message with the summary.
This way you will not append an item to History.

[mingmingl]

1. Change NoopStatistic to use uint64_t, to use consistent type for TrackingStatistic and NoopStatistic.
   • If type isn't consistent, a user of Statistic class would compile when using one statistic type but fail to compile when using the other statistic type. See example usage in https://github.com/llvm/llvm-project/blob/6dd094dd435d09872687c1bdcce6845f70cf25cc/llvm/lib/Transforms/Scalar/NewGVN.cpp#L3409

2. Update uint64_t usage in polly sub-projects accordingly.
   • This is caught by premerge-test, and not covered by previous ninja check-all when polly wasn't specified in DLLVM_ENABLE_PROJECTS

[mingmingl]

In D128070#3593257, @MaskRay wrote:

You may click Edit Revision to edit the summary and then run arc amend to update local commit message with the summary.
This way you will not append an item to History.

Acknowledged.

[nikic]

In D128070#3593045, @MaskRay wrote:

We check 64-bit atomic support (CMake check_working_cxx_atomics64), so the new use is fine. It may be first time LLVMSupport uses it, though (not checked very carefully).

Not just LLVMSupport, from what I can tell this is the first time LLVM requires libatomic -- previously this was only needed to work around compiler bugs (e.g. GCC targeting RISCV incorrectly used libatomic in some cases). That libatomic is now required for all 32-bit targets is quite unfortunate.

Is there some way that a consumer statically linking LLVM can detect which libraries they need to link when cross-compiling? llvm-config has this information, but obviously it doesn't work when cross-compiling. The cmake exports don't seem to have the information (and having to parse cmake files to extract this would certainly be not great).

llvm/include/llvm/ADT/Statistic.h

 class StringRef;
 
 class TrackingStatistic {
 public:
   const char *const DebugType;
   const char *const Name;
   const char *const Desc;
 
-  std::atomic<unsigned> Value;
+  std::atomic<uint64_t> Value;
   std::atomic<bool> Initialized;
 
   constexpr TrackingStatistic(const char *DebugType, const char *Name,
                               const char *Desc)
       : DebugType(DebugType), Name(Name), Desc(Desc), Value(0),
         Initialized(false) {}
 
   const char *getDebugType() const { return DebugType; }
   const char *getName() const { return Name; }
   const char *getDesc() const { return Desc; }
 
-  unsigned getValue() const { return Value.load(std::memory_order_relaxed); }
+  uint64_t getValue() const { return Value.load(std::memory_order_relaxed); }
 
   // Allow use of this class as the value itself.
-  operator unsigned() const { return getValue(); }
+  operator uint64_t() const { return getValue(); }
 
-  const TrackingStatistic &operator=(unsigned Val) {
+  const TrackingStatistic &operator=(uint64_t Val) {
     Value.store(Val, std::memory_order_relaxed);
     return init();
   }
 
   const TrackingStatistic &operator++() {
     Value.fetch_add(1, std::memory_order_relaxed);
     return init();
   }
 
   unsigned operator++(int) {
     init();
     return Value.fetch_add(1, std::memory_order_relaxed);
   }
 
   const TrackingStatistic &operator--() {
     Value.fetch_sub(1, std::memory_order_relaxed);
     return init();
   }
 
   unsigned operator--(int) {
     init();
     return Value.fetch_sub(1, std::memory_order_relaxed);
   }
 
-  const TrackingStatistic &operator+=(unsigned V) {
+  const TrackingStatistic &operator+=(uint64_t V) {
     if (V == 0)
       return *this;
     Value.fetch_add(V, std::memory_order_relaxed);
     return init();
   }
 
-  const TrackingStatistic &operator-=(unsigned V) {
+  const TrackingStatistic &operator-=(uint64_t V) {
     if (V == 0)
       return *this;
     Value.fetch_sub(V, std::memory_order_relaxed);
     return init();
   }
 
-  void updateMax(unsigned V) {
-    unsigned PrevMax = Value.load(std::memory_order_relaxed);
+  void updateMax(uint64_t V) {
+    uint64_t PrevMax = Value.load(std::memory_order_relaxed);
     // Keep trying to update max until we succeed or another thread produces
     // a bigger max than us.
     while (V > PrevMax && !Value.compare_exchange_weak(
                               PrevMax, V, std::memory_order_relaxed)) {
     }
     init();
   }
 

 
 /// Get the statistics. This can be used to look up the value of
 /// statistics without needing to parse JSON.
 ///
 /// This function does not prevent statistics being updated by other threads
 /// during it's execution. It will return the value at the point that it is
 /// read. However, it will prevent new statistics from registering until it
 /// completes.
-const std::vector<std::pair<StringRef, unsigned>> GetStatistics();
+const std::vector<std::pair<StringRef, uint64_t>> GetStatistics();
 
 /// Reset the statistics. This can be used to zero and de-register the
 /// statistics in order to measure a compilation.
 ///
 /// When this function begins to call destructors prior to returning, all
 /// statistics will be zero and unregistered. However, that might not remain the
 /// case by the time this function finishes returning. Whether update from other
 /// threads are lost or merely deferred until during the function return is

llvm/lib/Support/Statistic.cpp

void llvm::PrintStatistics(raw_ostream &OS) {
 
   // Print out the statistics header...
   OS << "===" << std::string(73, '-') << "===\n"
      << "                          ... Statistics Collected ...\n"
      << "===" << std::string(73, '-') << "===\n\n";
 
   // Print all of the statistics.
   for (TrackingStatistic *Stat : Stats.Stats)
-    OS << format("%*u %-*s - %s\n", MaxValLen, Stat->getValue(),
+    OS << format("%*" PRIu64 " %-*s - %s\n", MaxValLen, Stat->getValue(),
                  MaxDebugTypeLen, Stat->getDebugType(), Stat->getDesc());
 
   OS << '\n';  // Flush the output stream.
   OS.flush();
 }
 
 void llvm::PrintStatisticsJSON(raw_ostream &OS) {
   sys::SmartScopedLock<true> Reader(*StatLock);
if (EnableStats) {
     // Get the stream to write to.
     std::unique_ptr<raw_ostream> OutStream = CreateInfoOutputFile();
     (*OutStream) << "Statistics are disabled.  "
                  << "Build with asserts or with -DLLVM_FORCE_ENABLE_STATS\n";
   }
 #endif
 }
 
-const std::vector<std::pair<StringRef, unsigned>> llvm::GetStatistics() {
+const std::vector<std::pair<StringRef, uint64_t>> llvm::GetStatistics() {
   sys::SmartScopedLock<true> Reader(*StatLock);
-  std::vector<std::pair<StringRef, unsigned>> ReturnStats;
+  std::vector<std::pair<StringRef, uint64_t>> ReturnStats;
 
   for (const auto &Stat : StatInfo->statistics())
     ReturnStats.emplace_back(Stat->getName(), Stat->getValue());
   return ReturnStats;
 }
 
 void llvm::ResetStatistics() {
   StatInfo->reset();
 }

llvm/lib/Transforms/Scalar/NewGVN.cpp

#ifndef NDEBUG
   }
 #endif
 }
 
 // This is the main value numbering loop, it iterates over the initial touched
 // instruction set, propagating value numbers, marking things touched, etc,
 // until the set of touched instructions is completely empty.
 void NewGVN::iterateTouchedInstructions() {
-  unsigned int Iterations = 0;
+  uint64_t Iterations = 0;
   // Figure out where touchedinstructions starts
   int FirstInstr = TouchedInstructions.find_first();
   // Nothing set, nothing to iterate, just return.
   if (FirstInstr == -1)
     return;
   const BasicBlock *LastBlock = getBlockForValue(InstrFromDFSNum(FirstInstr));
   while (TouchedInstructions.any()) {
     ++Iterations;

llvm/unittests/ADT/StatisticTest.cpp

 //===- llvm/unittest/ADT/StatisticTest.cpp - Statistic unit tests ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 using namespace llvm;
 
-using OptionalStatistic = Optional<std::pair<StringRef, unsigned>>;
+using OptionalStatistic = Optional<std::pair<StringRef, uint64_t>>;
 
 namespace {
 #define DEBUG_TYPE "unittest"
 STATISTIC(Counter, "Counts things");
 STATISTIC(Counter2, "Counts other things");
 ALWAYS_ENABLED_STATISTIC(AlwaysCounter, "Counts things always");
 
 #if LLVM_ENABLE_STATS
 static void
-extractCounters(const std::vector<std::pair<StringRef, unsigned>> &Range,
+extractCounters(const std::vector<std::pair<StringRef, uint64_t>> &Range,
                 OptionalStatistic &S1, OptionalStatistic &S2) {
   for (const auto &S : Range) {
     if (S.first == "Counter")
       S1 = S;
     if (S.first == "Counter2")
       S2 = S;
   }
 }
 #endif
 
 TEST(StatisticTest, Count) {
   EnableStatistics();
 
   Counter = 0;
-  EXPECT_EQ(Counter, 0u);
+  EXPECT_EQ(Counter, 0ull);
   Counter++;
   Counter++;
+  Counter += (std::numeric_limits<uint64_t>::max() - 3);
 #if LLVM_ENABLE_STATS
-  EXPECT_EQ(Counter, 2u);
+  EXPECT_EQ(Counter, std::numeric_limits<uint64_t>::max() - 1);
 #else
-  EXPECT_EQ(Counter, 0u);
+  EXPECT_EQ(Counter, UINT64_C(0));
 #endif
 
   AlwaysCounter = 0;
-  EXPECT_EQ(AlwaysCounter, 0u);
+  EXPECT_EQ(AlwaysCounter, 0ull);
   AlwaysCounter++;
   ++AlwaysCounter;
-  EXPECT_EQ(AlwaysCounter, 2u);
+  EXPECT_EQ(AlwaysCounter, 2ull);
 }
 
 TEST(StatisticTest, Assign) {
   EnableStatistics();
 
   Counter = 2;
 #if LLVM_ENABLE_STATS
   EXPECT_EQ(Counter, 2u);

mlir/lib/Pass/PassStatistics.cpp

 
 constexpr StringLiteral kPassStatsDescription =
     "... Pass statistics report ...";
 
 namespace {
 /// Information pertaining to a specific statistic.
 struct Statistic {
   const char *name, *desc;
-  unsigned value;
+  uint64_t value;
 };
 } // namespace
 
 /// Utility to print a pass entry in the statistics output.
 static void printPassEntry(raw_ostream &os, unsigned indent, StringRef pass,
                            MutableArrayRef<Statistic> stats = llvm::None) {
   os.indent(indent) << pass << "\n";
   if (stats.empty())