[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 inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:164
+  volatile int *P = &V;
+  time_t Limit = time(NULL) + /*short wait*/ 20;
+
----------------
time_t is usually the number of seconds since the Unix epoch, so this might be (in terms of CPU time) a very long wait. Something between 1-10 milliseconds should be sufficient).

This precision is not possible with time(), so we'd have to use the <chrono> library from the C++ standard, possibly the TimePoint type from llvm/include/llvm/Support/Chrono.h.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:209
+  llvm::SmallVector<int64_t, 4> CycleArray;
+  std::unique_ptr<char[]> DataBuf(new char[kDataBufferSize]);
+  int NumTimeouts = 0;
----------------
A more C++14 way would be
auto DataBuf = std::make_unique<char[]>(kDataBufferSize);


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:45
+  asm volatile("" : "+m"(const_cast<T &>(var)));
+}
+
----------------
oontvoo wrote:
> 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?
> 
Normally, it would be built by the C++ compiler available on the host and selected during the configuration of LLVM build. Depending on the host system, it might be Clang, GCC, MSVC++ or others.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:167
+             "present. Exit with non-zero, otherwise"),
+    cl::cat(Options), cl::init(""));
 
----------------
oontvoo wrote:
> 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.
> 
> 
If the check that all the requires features are supported isn't too expensive (which it shouldn't be with the millisecond time limit), I'd prefer running it every time as a safety measure.

As for the flag - I think the main problem we need to solve here is that LBR tests will fail if we run them on hardware that doesn't support LBR timings (i.e. if they run on anything beyond somewhat recent Intel CPUs) or on a system that doesn't have LBR support.

I'm not sure that the flag would really help us - what we probably need one of the following:
- a flag that tests for the presence of the required features on the host, and that fails if and only if the needed features are not available (but it would never fail for any other reason). We'd also have to instrument the tests to first run this feature autodetection.
- a flag that makes llvm-exegesis "succeed" (i.e. end with exit code 0, though they might produce a message that informs the user that the test was skipped) on hosts where the features are not supported, so that the tests turn into a no-op.

There might be other ways to do this, but these two look like the simplest ones to implement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85254