[PATCH] D128070: [Support] Change TrackingStatistic to use uint64_t instead of unsigned
Mingming Liu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list