[libc-commits] [PATCH] D111624: [libc] automemcpy - result analyzer

Clement Courbet via Phabricator via libc-commits libc-commits at lists.llvm.org
Wed Oct 20 08:19:59 PDT 2021

courbet added a comment.

As discussed, this is complex enough to warrant unit tests.

Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:11
+// - Reads one or more json files.
+// - If there are several runs for the same function and distribution, picks the
+//   median throughput (aka `BytesPerSecond`).
You have not defined `distribution in this file yet`. Give a pointer ?

Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:195
+    auto &Values = Pair.second;
+    std::sort(Values.begin(), Values.end());
+    const double MedianValue = Values[Values.size() / 2];
`std::nth_element`, given that you're not using the values for anything else ?

Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:211
+// For normalize the function's throughput per distribution and assign a
+// normalized score to the function for a particular distribution.
Normalize ?

Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:160-162
+  StringMap<double> Throughputs; // Median of samples per distribution
+  StringMap<double> Scores;      // Normalized score per distribution
+  StringMap<Grade::GradeEnum> GradeAttribution; // Grade per distribution
gchatelet wrote:
> courbet wrote:
> > any reason not to group `{Throughput,Score,GradeAttribution}` to avoid the 3 parallel maps ?
> Yes, `Throughputs` is currently move constructed (see line 185).
> I can extract the distribution data into its own data structure with 3 fields, but then I'll have to iterate the map to construct `Throughputs` instead of moving it.
> I'll update the patch with the new version so you can judge.
I think it's better grouped.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list