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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/Target.h:148
+  virtual Error checkFeatureSupport(StringRef /*Unused*/) const {
+    return Error::success();
+  }
----------------
ondrasej wrote:
> 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.
Done. refactored it so that the counter check for the features depending on which mode is requested .
Specifically, if the target sees that lbr is requested, then it'll check for LBR otherwise, assume it's fine to run benchmarks


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:45
+  asm volatile("" : "+m"(const_cast<T &>(var)));
+}
+
----------------
ondrasej wrote:
> 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.
Hmm, can we not assume llvm-exegesis will always be built with clang?



================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:167
+             "present. Exit with non-zero, otherwise"),
+    cl::cat(Options), cl::init(""));
 
----------------
ondrasej wrote:
> 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.
How about this? We change it to a bool flag to say whether a smoke check should be done prior to running the benchmarks
If the flag is set, then then ask the target and respective counter(s) to do a check of hardware/software.
If flag is NOT set, then  assume support is present.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85254