[PATCH] D140271: [NFCI][llvm-exegesis] Benchmark: parallelize codegen (5x ... 8x less wallclock)

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:218-219
+    ThreadCount("j",
+                cl::desc("certain operations can be parallelized without "
+                         "sacrificing correctness. this specifies the number "
+                         "of threads to be used (default = 0 (autodetect))"),
----------------
Does this bring anything to the user?
Maybe "The number of threads to use for parallel operations (default = 0 (autodetect))"


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:225-228
+    cl::desc("sometimes it is not optimal to perform all parallel operations, "
+             "but only some, then do sequential operations, and repeat. this "
+             "sets the per-thread amount of operations we process per each "
+             "parallel section (default = 0 (autodetect))"),
----------------
"The batch size for parallel operations as it is not efficient to run one task per thread (default = 0 (autodetect))" ?


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:359
+  using ExpectedRunnableConfiguration =
+      std::optional<Expected<exegesis::BenchmarkRunner::RunnableConfiguration>>;
+  SmallVector<SmallVector<ExpectedRunnableConfiguration, MaxRepetitors>, 1>
----------------
I think we can drop the `exegesis` namespace qualifier here.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:367-372
+  // We must have that many things to do for *each* thread.
+  NumConfigurationsPerBatch *= Pool.getThreadCount(); // saturating
+  // But don't overcommit in presence of multiple repetitors.
+  NumConfigurationsPerBatch =
+      divideCeil(NumConfigurationsPerBatch, Repetitors.size());
+  assert(NumConfigurationsPerBatch > 0 && "Not processing anything!");
----------------
It is not clear to me what you're trying to achieve here.
I would introduce a function and assign the variable once instead of mutating it in place. The function name may also help understand what the intent is.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:381
+  // Outermost loop: run until we've processed all configurations.
+  while (!Configurations.empty()) {
+    // In each iteration, we deal with NumConfigurationsPerBatch-sized chunks.
----------------
Can we introduce functions to cut on the nesting level?

Also I believe function will largely improve readability.

```
while (!Configurations.empty()) {
  // setup PerConfigRCs
  computeBatch(PerConfigRCs);
  runBatch(PerConfigRCs);
  PerConfigRCs.clear();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140271