Make log functions variadic templates

[rocallahan]

What are the reasons/motivation for this change?

This makes all log-related functions support std::string-related types directly, leveraging the work in PR #5221 which does this for stringf(). This PR also adds support for std::string_view --- this is notable because printing string_views using old-school varargs functions is hard because they're not null-terminated.

After this lands, a lot of .c_str() boilerplate can be removed all over Yosys. It also enables fixing #5215 and #5210 in a clean way, because we can change log_signal() etc to return a temporary object or a string reference and not have to add .c_str().

Support for more types, e.g. Yosys-specific types like IdString, could be easily added in the future. This also creates opportunities to improve performance (beyond what can be achieved with the old varargs implementation) if that matters.

Explain how this is achieved.

Simple refactorings of each log-related function. Very few call sites needed to be updated (in seven places non-constant strings were being used for the log() format), so I think we can consider this source-compatible. Also when building with C++17 (the default) non-constant formats would still compile.

Note that this builds PR #5221; the first two commits belong to that PR. Like that PR, this probably needs to land in conjunction with an update to the Verific prebuilt library.

If applicable, please suggest to reviewers how they can test the change.

Build with CXXSTD=c++20 to get compile-time checking of the format strings. That should probably be enabled in CI after this lands.

[widlarizer]

Somehow, this too adds 1-2% runtime to the jpeg synthesis perf test, even on C++20 with LTO

[rocallahan]

Somehow, this too adds 1-2% runtime to the jpeg synthesis perf test, even on C++20 with LTO

I'm not seeing this. I'm seeing a small speedup instead (default build with C++17 and no LTO).

rocallahan@rocallahan:~/yosys$ git checkout main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
rocallahan@rocallahan:~/yosys$ make clean > /dev/null
rocallahan@rocallahan:~/yosys$ make -j48 > /dev/null
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):     115.2 ms ±  92.7 ms    [User: 71.9 ms, System: 43.1 ms]
  Range (min … max):    84.4 ms … 379.0 ms    10 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (379.0 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      86.9 ms ±   0.9 ms    [User: 71.9 ms, System: 14.9 ms]
  Range (min … max):    85.1 ms …  89.0 ms    33 runs
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      86.2 ms ±   0.9 ms    [User: 71.0 ms, System: 15.1 ms]
  Range (min … max):    84.1 ms …  87.6 ms    33 runs
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      86.1 ms ±   1.0 ms    [User: 71.2 ms, System: 14.8 ms]
  Range (min … max):    84.2 ms …  88.5 ms    33 runs
 
rocallahan@rocallahan:~/yosys$ git checkout typed-log
Switched to branch 'typed-log'
Your branch and 'rocallahan/typed-log' have diverged,
and have 207 and 11 different commits each, respectively.
  (use "git pull" if you want to integrate the remote branch with yours)
rocallahan@rocallahan:~/yosys$ make -j48 > /dev/null
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):     117.4 ms ± 100.1 ms    [User: 71.3 ms, System: 46.0 ms]
  Range (min … max):    84.2 ms … 402.3 ms    10 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (402.3 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      85.7 ms ±   1.1 ms    [User: 73.1 ms, System: 12.5 ms]
  Range (min … max):    83.6 ms …  89.8 ms    34 runs
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      85.5 ms ±   0.8 ms    [User: 71.6 ms, System: 13.8 ms]
  Range (min … max):    83.6 ms …  86.9 ms    33 runs
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      85.3 ms ±   0.7 ms    [User: 73.0 ms, System: 12.2 ms]
  Range (min … max):    83.8 ms …  86.9 ms    33 runs
 
rocallahan@rocallahan:~/yosys$ hyperfine './yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"'
Benchmark 1: ./yosys -dp "read_rtlil /usr/local/google/home/rocallahan/Downloads/jpeg.synth.il; dump"
  Time (mean ± σ):      85.2 ms ±   1.0 ms    [User: 70.1 ms, System: 14.9 ms]
  Range (min … max):    83.5 ms …  88.6 ms    34 runs

Can you recheck to make sure you made the comparison the right way around?

[rocallahan]

I've updated this PR to have the log() functions take parameters by reference. We'll need this when we remove .c_str()s to log std::string and IdString directly.

That makes this PR performance-neutral for me instead of a small improvement. Interestingly, just converting log() is still a performance improvement. The improvement goes away with the commit "Make log_error() use variadic templates", which makes absolutely no sense. Maybe we are hitting some weird effects due to code sizes or inlining heuristics or something like that.

[rocallahan]

OK, I'm not 100% sure what I'm doing here because the performance differences I'm seeing are pretty close to the noise floor on my machines, but I'm pretty sure that the commit converting log_error() was a 1% regression or so, and that Move log_assert_worker()'s call to log_error() into an out-of-line non-varags function and Move log_abort()'s call to log_error() into an out-of-line non-varargs function mitigate that regression by turning a lot of variadic log_error() calls into simple non-templated function calls... which presumably improves codegen/code layout a bit.

[rocallahan]

I'm not sure what's up with the Ubuntu test failure; tests are passing for me.

[jix]

The failure for the "with Verific" builds is due to some of the logging functions being called from the non-public verific integration and the CI is using a cached build of those. We most likely also need to make coordinated changes to those non-public parts. I will prepare those, test the combination locally and report back.

[jix]

Instead of making those coordinated changes, I decided to add kernel/log_compat.cc to maintain backwards ABI compatibility with the functions that are called from the YosysHQ Verific Extensions. This is less disruptive to our internal workflows and we can remove it at some later point when we are ready to update the extensions to the new logging functions. None of log_compat.cc's definitions are exposed in any public header, so there should be no issue with something else starting to depend on them.

Also note that a C++20 build is already part of the "Compiler testing" workflow.