[PATCH] D97504: [llvm-exegesis] Disable the LBR check on AMD

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added inline comments.


================
Comment at: llvm/include/llvm/Support/Host.h:73
+  namespace x86 {
+  enum VendorSignatures {
+    SIG_INTEL = 0x756e6547 /* Genu */,
----------------
Why not "enum class"? With that, we would not need the "SIG_" prefix on all the value names.


================
Comment at: llvm/lib/Support/Host.cpp:502
+VendorSignatures getVendorSignature() {
+  unsigned __attribute__((unused)) EAX = 0, EBX = 0, ECX = 0, EDX = 0;
+  unsigned MaxLeaf, Vendor;
----------------
mgorny wrote:
> Smells a bit weird that you're declaring EAX and EBX if you never intend to use it.
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 [[ https://en.wikipedia.org/wiki/CPUID#EAX=0:_Highest_Function_Parameter_and_Manufacturer_ID | 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.


================
Comment at: llvm/lib/Support/Host.cpp:518-519
+} // namespace llvm
+using namespace llvm::sys::detail::x86;
+using llvm::sys::detail::x86::VendorSignatures;
+
----------------
Are the using statements actually needed? It seems that the function or the enum class is never used in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97504