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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: llvm/include/llvm/Support/Host.h:73
+  namespace x86 {
+  enum VendorSignatures {
+    SIG_INTEL = 0x756e6547 /* Genu */,
----------------
ondrasej wrote:
> Why not "enum class"? With that, we would not need the "SIG_" prefix on all the value names.
Done. Made this an `enum class`  and renamed the members.


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


Removed. 



================
Comment at: llvm/lib/Support/Host.cpp:502
+VendorSignatures getVendorSignature() {
+  unsigned __attribute__((unused)) EAX = 0, EBX = 0, ECX = 0, EDX = 0;
+  unsigned MaxLeaf, Vendor;
----------------
oontvoo wrote:
> ondrasej wrote:
> > 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.
> > Smells a bit weird that you're declaring EAX and EBX if you never intend to use it.
> 
> 
> Removed. 
> 
Thanks! I've gone with 3) since it's easier to use (we won't need string comparisons).




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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97504