[llvm-exegesis] Disable the LBR check on AMD

[oontvoo]

Summary

https://bugs.llvm.org/show_bug.cgi?id=48918

The bug reported a hang (or very very slow runtime) on a Zen2. Unfortunately, we don't have the hardware right now to debug it and I was not able to reproduce the bug on a HSW.
Theory we've got is that the lbr-checking code could be confused on AMD.

[oontvoo]

cleanup

[mgorny]

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

751

I wonder if it wouldn't be better to explicitly check for Intel here, or maybe even for Intel family new enough for LBR. I'm going to guess that Cyrix/VIA processors don't have it either.

[ondrasej]

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

751

+1. The perf counters are vendor specific (in particular, we have some Intel-specific magic numbers in the code), so it would make more sense to allow only Intel rather than reject only AMD. We can allow more architectures as soon as we can verify the LBR support on them.

[oontvoo]

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

751

Good point.
So ideally we want to say abort if we see anything that's not Intel, but looking at llvm::Triple::VendorType, I don't see Intel listed as a vendor.
A bit surprised... Does anyone know why?

[mgorny]

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

751

Probably because nobody lists CPU vendor in the vendor field. It's usually pc or unknown.

IIRC the code is going to run on host, so you probably want to look at CPUID for the CPU vendor. sys::getHostCPUName has some logic to check for Intel/AMD but doesn't seem to return that usefully. Maybe you should copy the part of that logic, i.e. something like:

if (getX86CpuIDAndInfo(0, &MaxLeaf, &Vendor, &ECX, &EDX) || MaxLeaf < 1 || Vendor != 0x756e6547)
  return; // not intel

[oontvoo]

Updated diff: Add a util in Host.h to check the host CPU's vendor.

[mgorny]

llvm/lib/Support/Host.cpp

502

Smells a bit weird that you're declaring EAX and EBX if you never intend to use it.

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

738

Unless I'm missing something, state is no longer necessary here.

[ondrasej]

llvm/include/llvm/Support/Host.h

73

Why not "enum class"? With that, we would not need the "SIG_" prefix on all the value names.

llvm/lib/Support/Host.cpp

502

The vendor ID strings span over EBX, EDX, and ECX (for Intel, it spells "GenuineIntel"), and we should check all three of them, not just the first 32 bits, because this might be ambiguous (according to Wikipedia, there are non-Intel chips where the text also starts with "Genu" - even though they are probably old Intel clones).

I think we have two options here:

1. Remove VendorSignatures and return an Optional<std::string> (because the signature is a human-readable string anyway) and provide constants for the most common vendor strings,
2. Instead of returning a string, return a fixed size structure (a 12-byte array or a struct with fields for EBX, EDX, ECX),
3. Keep VendorSignatures, and add one value for each known signature (UNKNOWN, GENUINE_INTEL for "GenuineIntel", AUTHENTIC_AMD for "AuthenticAMD", ...) do the proper string matching inside getVendorSignature() and let people add new signatures as they need them.

I'd strongly prefer 1 (all the known vendor signatures are human readable strings anyway) or 3 (easier use on caller side), option 2 makes the use of the signature on caller side more difficult.

518–519

Are the using statements actually needed? It seems that the function or the enum class is never used in this file.

[oontvoo]

Updated diff:
Check the full sig string rather than just the first 4 bytes.

llvm/include/llvm/Support/Host.h

73

Done. Made this an enum class and renamed the members.

llvm/lib/Support/Host.cpp

502

Smells a bit weird that you're declaring EAX and EBX if you never intend to use it.

Removed.

502

Thanks! I've gone with 3) since it's easier to use (we won't need string comparisons).

518–519

The VendorSignatures enum are used. They used to be declared locally to this cpp file, but I've moved it to the header. I didn't want to update all the references due to the change in its namespace, so I'd added this` using`.

I've removed the using VendorSignatures That is not needed now that we've switched to an enum class.
The using namespace is still needed to avoid having to spell out the full ns in other places in this file.

[mgorny]

Can't say how it works on Intel but I can confirm that the problem is gone on AMD.

[ondrasej]

One more const-comment, otherwise looks good.

llvm/lib/Support/Host.cpp

1128

Can this be const?

[oontvoo]

Updated diff:

• make Vendor const
• move var decls closer to where they are used and made clang-diagnostic-uninitialized happy

[oontvoo]

In D97504#2602514, @mgorny wrote:

Can't say how it works on Intel but I can confirm that the problem is gone on AMD.

Thanks for confirming it for AMD! I've tested it on Intel.

[oontvoo]

rebase

[thakis]

This doesn't build on at least mac/arm hosts: http://45.33.8.238/macm1/4600/step_3.txt

[thakis]

In D97504#2603795, @thakis wrote:

This doesn't build on at least mac/arm hosts: http://45.33.8.238/macm1/4600/step_3.txt

From looking at the code, I think this is broken on all non-intel hosts. Reverted for now in 76148caa505c85e5ea5e57d67f1f618f59d7e86f

[oontvoo]

In D97504#2603805, @thakis wrote:

In D97504#2603795, @thakis wrote:

This doesn't build on at least mac/arm hosts: http://45.33.8.238/macm1/4600/step_3.txt

From looking at the code, I think this is broken on all non-intel hosts. Reverted for now in 76148caa505c85e5ea5e57d67f1f618f59d7e86f

hmm from the build log, this is failing to compile an x86-specific target (`llvm-exegesis/lib/X86/target.cpp) on an aarch64. That seems weird.

[oontvoo]

Updated diff with fix for non-x86 platforms

[lebedev.ri]

This is still a preprocessor mess that won't compile, i think.
getVendorSignature() should always exist, but if not being compiled for/on X86, it should return unknown.

[oontvoo]

Updated diff: defined getVendorSignature() for the non-x86 case too.

[oontvoo]

In D97504#2604040, @lebedev.ri wrote:

This is still a preprocessor mess that won't compile, i think.
getVendorSignature() should always exist, but if not being compiled for/on X86, it should return unknown.

Done

[lebedev.ri]

This seems about right now, thanks.

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

750–751

I don't think you need this any more

[ondrasej]

llvm/lib/Support/Host.cpp

507

As noted in the other patch - when MaxLeaf is nullptr we should set it to zero if this return statement is used.

Otherwise, it will keep its default value and for the caller it will be impossible to distinguish this branch from the return statement on line 520.

[oontvoo]

init MaxLeaf to 0

llvm/lib/Support/Host.cpp

507

Done. (Setting MaxLeaf to 0 when it's not null in the check on line 503)

[ondrasej]

Looks good except for the typo.

llvm/lib/Support/Host.cpp

505

This looks like a typo. Also, please move the assignment on the following line in the interest of readability.

[oontvoo]

Fixed typo

[oontvoo]

Thanks all for the review!

llvm/lib/Support/Host.cpp

505

Good catch!

[ppenzin]

Apologies if this is not the right channel to raise issues with the patch.

Order of registers in vendor string looks off, and I can see this patch breaking a project I am building.

Debugger walkthrough on AMD A10 running FreeBSD:

(gdb) 
515	  if (EBX == 0x756e6547 && ECX == 0x6c65746e && EDX == 0x49656e69)
(gdb) 
519	  if (EBX == 0x68747541 && ECX == 0x69746e65 && EDX == 0x444d4163)
(gdb) 
522	  return VendorSignatures::UNKNOWN;
(gdb) p/x EBX
$2 = 0x68747541
(gdb) p/x ECX
$3 = 0x444d4163
(gdb) p/x EDX
$4 = 0x69746e65

I think the latter two registers are in wrong order (see inline comment). A curious bit is that the old code did not use those.

llvm/lib/Support/Host.cpp

514–519

I think you have ECX and EDX flipped around, vendor string supposed to be read from EBX, EDX, ECX (in order): https://en.wikipedia.org/wiki/CPUID#EAX=0:_Highest_Function_Parameter_and_Manufacturer_ID

I can observe this patch returning UNKNOWN instead of AUTHENTIC_AMD on AMD A10 running FreeBSD.

[oontvoo]

llvm/lib/Support/Host.cpp

514–519

Thanks! The Intel case was correct. ( ECX went first, but its expected value is 0x6c65746e, which is ntel)
I've sent to correct the AMD case.https://reviews.llvm.org/D98322

llvm/include/llvm/Support/Host.h

namespace sys {
   int getHostNumPhysicalCores();
 
   namespace detail {
   /// Helper functions to extract HostCPUName from /proc/cpuinfo on linux.
   StringRef getHostCPUNameForPowerPC(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForARM(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForS390x(StringRef ProcCpuinfoContent);
   StringRef getHostCPUNameForBPF();
+
+  /// Helper functions to extract CPU details from CPUID on x86.
+#if defined(__i386__) || defined(_M_IX86) || defined(__x86_64__) ||            \
+    defined(_M_X64)
+  namespace x86 {
+  enum class VendorSignatures {
+    UNKNOWN,
+    GENUINE_INTEL,
+    AUTHENTIC_AMD,
+  };
+
+  /// Returns the host CPU's vendor.
+  /// MaxLeaf: if a non-nullptr pointer is specified, the EAX value will be
+  /// assigned to its pointee.
+  VendorSignatures getVendorSignature(unsigned *MaxLeaf = nullptr);
+  } // namespace x86
+#endif
   }
 }
 }
 
 #endif

llvm/lib/Support/Host.cpp

#else
   }
   return "v1";
 #endif
 }
 
 #if defined(__i386__) || defined(_M_IX86) || \
     defined(__x86_64__) || defined(_M_X64)
 
-enum VendorSignatures {
-  SIG_INTEL = 0x756e6547 /* Genu */,
-  SIG_AMD = 0x68747541 /* Auth */
-};
-
 // The check below for i386 was copied from clang's cpuid.h (__get_cpuid_max).
 // Check motivated by bug reports for OpenSSL crashing on CPUs without CPUID
 // support. Consequently, for i386, the presence of CPUID is checked first
 // via the corresponding eflags bit.
 // Removal of cpuid.h header motivated by PR30384
 // Header cpuid.h and method __get_cpuid_max are not used in llvm, clang, openmp
 // or test-suite, but are used in external projects e.g. libstdcxx
 static bool isCpuIdSupported() {
#elif defined(_MSC_VER)
   *rECX = registers[2];
   *rEDX = registers[3];
   return false;
 #else
   return true;
 #endif
 }
 
+namespace llvm {
+namespace sys {
+namespace detail {
+namespace x86 {
+
+VendorSignatures getVendorSignature(unsigned *MaxLeaf) {
+  unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
+  if (MaxLeaf == nullptr)
+    MaxLeaf = &EAX;
+
+  if (!isCpuIdSupported())
+    return VendorSignatures::UNKNOWN;
+
+  if (getX86CpuIDAndInfo(0, MaxLeaf, &EBX, &ECX, &EDX) || *MaxLeaf < 1)
+    return VendorSignatures::UNKNOWN;
+
+  // "Genu ineI ntel"
+  if (EBX == 0x756e6547 && ECX == 0x6c65746e && EDX == 0x49656e69)
+    return VendorSignatures::GENUINE_INTEL;
+
+  // "Auth enti cAMD"
+  if (EBX == 0x68747541 && ECX == 0x69746e65 && EDX == 0x444d4163)
+    return VendorSignatures::AUTHENTIC_AMD;
+
+  return VendorSignatures::UNKNOWN;
+}
+
+} // namespace x86
+} // namespace detail
+} // namespace sys
+} // namespace llvm
+
+using namespace llvm::sys::detail::x86;
+
 /// getX86CpuIDAndInfoEx - Execute the specified cpuid with subleaf and return
 /// the 4 values in the specified arguments.  If we can't run cpuid on the host,
 /// return true.
 static bool getX86CpuIDAndInfoEx(unsigned value, unsigned subleaf,
                                  unsigned *rEAX, unsigned *rEBX, unsigned *rECX,
                                  unsigned *rEDX) {
 #if defined(__GNUC__) || defined(__clang__)
 #if defined(__x86_64__)
#endif
   if (HasExtLeaf1 && ((ECX >> 16) & 1))
     setFeature(X86::FEATURE_FMA4);
 
   if (HasExtLeaf1 && ((EDX >> 29) & 1))
     setFeature(X86::FEATURE_64BIT);
 }
 
 StringRef sys::getHostCPUName() {
-  unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
-  unsigned MaxLeaf, Vendor;
-
-  if (!isCpuIdSupported())
+  unsigned MaxLeaf = 0;
+  const VendorSignatures Vendor = getVendorSignature(&MaxLeaf);
+  if (Vendor == VendorSignatures::UNKNOWN)
     return "generic";
 
-  if (getX86CpuIDAndInfo(0, &MaxLeaf, &Vendor, &ECX, &EDX) || MaxLeaf < 1)
-    return "generic";
+  unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
   getX86CpuIDAndInfo(0x1, &EAX, &EBX, &ECX, &EDX);
 
   unsigned Family = 0, Model = 0;
   unsigned Features[(X86::CPU_FEATURE_MAX + 31) / 32] = {0};
   detectX86FamilyModel(EAX, &Family, &Model);
   getAvailableFeatures(ECX, EDX, MaxLeaf, Features);
 
   // These aren't consumed in this file, but we try to keep some source code the
   // same or similar to compiler-rt.
   unsigned Type = 0;
   unsigned Subtype = 0;
 
   StringRef CPU;
 
-  if (Vendor == SIG_INTEL) {
+  if (Vendor == VendorSignatures::GENUINE_INTEL) {
     CPU = getIntelProcessorTypeAndSubtype(Family, Model, Features, &Type,
                                           &Subtype);
-  } else if (Vendor == SIG_AMD) {
+  } else if (Vendor == VendorSignatures::AUTHENTIC_AMD) {
     CPU = getAMDProcessorTypeAndSubtype(Family, Model, Features, &Type,
                                         &Subtype);
   }
 
   if (!CPU.empty())
     return CPU;
 
   return "generic";

llvm/tools/llvm-exegesis/lib/X86/Target.cpp

 #include "X86Counter.h"
 #include "X86RegisterInfo.h"
 #include "X86Subtarget.h"
 #include "llvm/ADT/Sequence.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Host.h"
 
 #include <memory>
 #include <string>
 #include <vector>
 #if defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
 #include <immintrin.h>
 #include <intrin.h>
 #endif
std::unique_ptr<SnippetGenerator> createParallelSnippetGenerator(
       const SnippetGenerator::Options &Opts) const override {
     return std::make_unique<X86ParallelSnippetGenerator>(State, Opts);
   }
 
   bool matchesArch(Triple::ArchType Arch) const override {
     return Arch == Triple::x86_64 || Arch == Triple::x86;
   }
 
   Error checkFeatureSupport() const override {
     // LBR is the only feature we conditionally support now.
     // So if LBR is not requested, then we should be able to run the benchmarks.
     if (LbrSamplingPeriod == 0)
       return Error::success();
 
 #if defined(__linux__) && defined(HAVE_LIBPFM) &&                              \
     defined(LIBPFM_HAS_FIELD_CYCLES)
+      // FIXME: Fix this.
+      // https://bugs.llvm.org/show_bug.cgi?id=48918
+      // For now, only do the check if we see an Intel machine because
+      // the counter uses some intel-specific magic and it could
+      // be confuse and think an AMD machine actually has LBR support.
+#if defined(__i386__) || defined(_M_IX86) || defined(__x86_64__) ||            \
+    defined(_M_X64)
+    using namespace sys::detail::x86;
+
+    if (getVendorSignature() == VendorSignatures::GENUINE_INTEL)
       // If the kernel supports it, the hardware still may not have it.
       return X86LbrCounter::checkLbrSupport();
-#else
+#endif
+#endif
     return llvm::make_error<llvm::StringError>(
         "LBR not supported on this kernel and/or platform",
         llvm::errc::not_supported);
-#endif
   }
 
   std::unique_ptr<SavedState> withSavedState() const override {
     return std::make_unique<X86SavedState>();
   }
 
   static const unsigned kUnavailableRegisters[4];
 };