[PATCH] D85254: [llvm-exegesis] Add option to check the hardware support for a given feature before benchmarking.

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added a comment.

Thanks Vy for working on this!

My main comments are about the feature flag + feature strings: rather than specifying the requested features through the command-line flag, we should auto-detect which features are required and then check for them appropriately (in part, we're already doing this with the uops counters, which are available only on certain platforms). Just this - and perhaps being able to show which features are supported by the host should be sufficient & give a better user experience.



================
Comment at: llvm/tools/llvm-exegesis/lib/Target.h:148
+  virtual Error checkFeatureSupport(StringRef /*Unused*/) const {
+    return Error::success();
+  }
----------------
We probably want to reverse this - make all features disabled by default and whitelist them only when we know they are available. We'll typically be adding a check when some feature is generally not available, so it would make more sense to disable it unless we can prove that it is available.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:677
 
+  Error checkFeatureSupport(StringRef FeatureName) const override {
+    // LBR is the only feature we conditionally support now.
----------------
I'd prefer to avoid passing around raw strings for feature checks - this would make it difficult to e.g. put together a list of all features that are available across all platforms (i.e. the list of valid inputs to this function), or the list of features that are available on the current host.

If we want to keep this generic function, I'd prefer having an enum type that enumerates all the possible features, and functions for parsing and string formatting that could be used in the command-line interface (more on the interface in the comments).

Then, for each target, we could list all the features (enum values) that are always supported, and a list of those that need a host-specific check (along with a function/object that checks for their presence).


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:45
+  asm volatile("" : "+m"(const_cast<T &>(var)));
+}
+
----------------
This is not portable - the inline assembly is compiler-specific, and this would not work e.g. with Microsoft's C++ compiler. Sadly, a portable implementation would include a lot of #ifdefs and different implementation - see for example [DoNotOptimize()](https://github.com/llvm/llvm-project/blob/master/llvm/utils/benchmark/include/benchmark/benchmark.h#L312) in the benchmark library.

The code looks quite complex and fragile, so I'd prefer not duplicating it. I'm wondering if we should depend on the benchmark library (which would mean some unexpected and possibly dangerous dependencies), or use another way to prevent the compiler from fully inlining this, e.g. make the number of iterations somehow depend on the host/the environment, e.g. the current time.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:157
 
+llvm::Error X86LbrCounter::CheckLbrSupport() {
+  // Do a sample read and check if the results contain non-zero values.
----------------
Style nit: checkLbrSupport


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:166
+  int Limit = 10 * (reinterpret_cast<size_t>(counter.MMappedBuffer) % 3);
+  int sum = 0;
+  for (int i = 0; i < Limit; ++i) {
----------------
Style nit: Sum


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:167
+  int sum = 0;
+  for (int i = 0; i < Limit; ++i) {
+    sum += i * i;
----------------
Style nit: I


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:174
+
+  auto ResultOrError = counter.doReadCounter(nullptr, nullptr);
+
----------------
Can this be const auto?


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:183
+  return llvm::make_error<llvm::StringError>(
+      "LBR probably not supported on this hardware", llvm::errc::not_supported);
+}
----------------
I'd prefer being more concrete here, e.g. "LBR format with cycles is not supported on the host", since we'll also return this error when the LBR is there, but it doesn't have the timing info (e.g. on Haswell).


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:167
+             "present. Exit with non-zero, otherwise"),
+    cl::cat(Options), cl::init(""));
 
----------------
I'd prefer removing the flag altogether. With this flag, we're effectively asking the user to tell us the CPU features that will be used by llvm-exegesis given the values of the other command-line flags, which is something we should be able to determine ourselves in the code from the other flags.

Rather than have this flag, we should recognize that the user is trying to use the LBR (or some other future feature) from other command-line flags, and run the check automatically for them. From the viewpoint of the user, this will be a significant reduction of complexity (they will not need to know if they have to check for some features), but the overall behavior of the program will remain the same (there will be a non-zero exit code if the feature is not supported).

What might make sense instead (though it might be dangerous too) is a flag that would make llvm-exegesis exit peacefully if the required feature is not there. We could use this to make tests no-ops on targets where the feature is not supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85254