[PATCH] D128070: [Support] Change TrackingStatistic to use uint64_t instead of unsigned

Fri Jun 17 13:00:50 PDT 2022

mingmingl marked an inline comment as done.
mingmingl added a comment.

In D128070#3592531 <https://reviews.llvm.org/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`

In D128070#3593045 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128070/new/

https://reviews.llvm.org/D128070