Differential D3575 Diff 15991 source/lib/sysdep/arch/x86_x64/topology.cpp

Changeset View

Standalone View

View Options

source/lib/sysdep/arch/x86_x64/topology.cpp

StanUnsubmitted

Not Done

2021

Stan: 2021

* *

* Permission is hereby granted, free of charge, to any person obtaining * Permission is hereby granted, free of charge, to any person obtaining

* a copy of this software and associated documentation files (the * a copy of this software and associated documentation files (the

* "Software"), to deal in the Software without restriction, including * "Software"), to deal in the Software without restriction, including

* without limitation the rights to use, copy, modify, merge, publish, * without limitation the rights to use, copy, modify, merge, publish,

* distribute, sublicense, and/or sell copies of the Software, and to * distribute, sublicense, and/or sell copies of the Software, and to

* permit persons to whom the Software is furnished to do so, subject to * permit persons to whom the Software is furnished to do so, subject to

* the following conditions: * the following conditions:

Show All 14 Lines

* Detection of CPU topology * Detection of CPU topology

*/ */

#include "precompiled.h" #include "precompiled.h"

#include "lib/sysdep/arch/x86_x64/topology.h" #include "lib/sysdep/arch/x86_x64/topology.h"

#include "lib/bits.h" #include "lib/bits.h"

#include "lib/module_init.h" #include "lib/module_init.h"

#include "lib/sysdep/cpu.h" // ERR::CPU_FEATURE_MISSING #include "lib/sysdep/cpu.h" // ERR::CPU_FEATURE_MISSING

StanUnsubmitted

Not Done

#include "lib/module_init.h"

+ #include "lib/sysdep/arch.h" // ARCH_IA32

#include "lib/sysdep/cpu.h" // ERR::CPU_FEATURE_MISSING

#include "lib/sysdep/os_cpu.h"

Stan:

#include "lib/sysdep/os_cpu.h" #include "lib/sysdep/os_cpu.h"

#include "lib/sysdep/numa.h" #include "lib/sysdep/numa.h"

#include "lib/sysdep/arch/x86_x64/x86_x64.h" #include "lib/sysdep/arch/x86_x64/x86_x64.h"

#include "lib/sysdep/arch/x86_x64/apic.h" #include "lib/sysdep/arch/x86_x64/apic.h"

#include <bitset> #include <bitset>

#include <vector> #include <vector>

Show All 34 Lines

static size_t MaxLogicalPerCore() static size_t MaxLogicalPerCore()

{ {

struct IsHyperthreadingCapable struct IsHyperthreadingCapable

{ {

bool operator()() const bool operator()() const

{ {

// definitely not // definitely not

if(!x86_x64::Cap(x86_x64::CAP_HT)) if(!x86_x64::Cap(x86_x64::CAP_HT))

return false; return false;

// multi-core AMD systems falsely set the HT bit for reasons of // multi-core AMD systems falsely set the HT bit for reasons of

// compatibility. we'll just ignore it, because clearing it might // compatibility. we'll just ignore it, because clearing it might

// confuse other callers. // confuse other callers.

if(x86_x64::Vendor() == x86_x64::VENDOR_AMD && x86_x64::Cap(x86_x64::CAP_AMD_CMP_LEGACY)) if(x86_x64::Vendor() == x86_x64::VENDOR_AMD && x86_x64::Cap(x86_x64::CAP_AMD_CMP_LEGACY))

return false; return false;

return true; return true;

StanUnsubmitted

Not Done

bool operator()() const

{

// definitely not

if(!x86_x64::Cap(x86_x64::CAP_HT))

return false;

+ #if ARCH_IA32

+ // first generation Intel multi-core systems without HT (Penryn,

+ // Wolfdale, Yorkfield) falsely set the HT bit

+ // its confused inline asm in x86_x64::__cpuid() for GCC+i386+PIC

+ // hide assert and disable hyperthreading

+ if(logicalPerPackage % maxCoresPerPackage && x86_x64::Vendor() == x86_x64::VENDOR_INTEL && x86_x64::Family() == 6)

+ return false;

+ #endif

// multi-core AMD systems falsely set the HT bit for reasons of

// compatibility. we'll just ignore it, because clearing it might

// confuse other callers.

if(x86_x64::Vendor() == x86_x64::VENDOR_AMD && x86_x64::Cap(x86_x64::CAP_AMD_CMP_LEGACY))

return false;

return true;

}

};

if(IsHyperthreadingCapable()())

Stan:

nwtourAuthorUnsubmitted

Done

logicalPerPackage and maxCoresPerPackage uninitialized here. Only in line 102-103 (After checking IsHyperthreadingCapable())

nwtour: logicalPerPackage and maxCoresPerPackage uninitialized here. Only in line 102-103 (After…

nwtourAuthorUnsubmitted

Done

nwtour:

} }

}; };

if(IsHyperthreadingCapable()()) if(IsHyperthreadingCapable()())

{ {

x86_x64::CpuidRegs regs = { 0 }; x86_x64::CpuidRegs regs = { 0 };

regs.eax = 1; regs.eax = 1;

if(!x86_x64::cpuid(&regs)) if(!x86_x64::cpuid(&regs))

DEBUG_WARN_ERR(ERR::CPU_FEATURE_MISSING); DEBUG_WARN_ERR(ERR::CPU_FEATURE_MISSING);

const size_t logicalPerPackage = bits(regs.ebx, 16, 23); const size_t logicalPerPackage = bits(regs.ebx, 16, 23);

const size_t maxCoresPerPackage = MaxCoresPerPackage(); const size_t maxCoresPerPackage = MaxCoresPerPackage();

#if defined(CPUID_IMPLEMENTATION_PIC86)

// first generation Intel multi-core systems without HT (Penryn,

vladislavbelovUnsubmitted

Not Done

According to GCC documentation CPU detection code might be affected by compiler arguments (like msse), which means that the whole code of the numbers detection might be incorrect.

https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/x86-Built-in-Functions.html#x86-Built-in-Functions:

If you specify command-line switches such as -msse, the compiler could use the extended instruction sets even if the built-ins are not used explicitly in the program. For this reason, applications that perform run-time CPU detection must compile separate files for each supported architecture, using the appropriate flags. In particular, the file containing the CPU detection code should be compiled without these options.

vladislavbelov: According to GCC documentation CPU detection code might be affected by compiler arguments (like…

// Wolfdale, Yorkfield) falsely set the HT bit

// its confused inline asm in x86_x64::__cpuid() for GCC+i386+PIC

// hide assert and disable hyperthreading

if(logicalPerPackage % maxCoresPerPackage && x86_x64::Vendor() == x86_x64::VENDOR_INTEL && x86_x64::Family() == 6)

{

return 1;

}

#endif

wraitiiUnsubmitted

Not Done

I think you should put this in the struct above. We already have a similar special case for AMD.

We also define a ARCH_IA32 define in arch.h - I'm not sure if that's more relevant here.

wraitii: I think you should put this in the struct above. We already have a similar special case for AMD.

nwtourAuthorUnsubmitted

Done

We already have a similar special case for AMD.

For AMD guaranteed method to check if a flag HT is false.

Immediately, I know only for some versions the code (gcc+386+PIC) and CPU (first generation core2duo) is probably incorrect and disable the assertion. This is different

nwtour: >We already have a similar special case for AMD. For AMD guaranteed method to check if a flag…

wraitiiUnsubmitted

Not Done

Right I disregarded the additional checks that you're doing already. I think the patch looks OK then.

wraitii: Right I disregarded the additional checks that you're doing already. I think the patch looks OK…

// cores ought to be uniform WRT # logical processors // cores ought to be uniform WRT # logical processors

ENSURE(logicalPerPackage % maxCoresPerPackage == 0); ENSURE(logicalPerPackage % maxCoresPerPackage == 0);

const size_t maxLogicalPerCore = logicalPerPackage / maxCoresPerPackage; const size_t maxLogicalPerCore = logicalPerPackage / maxCoresPerPackage;

return maxLogicalPerCore; return maxLogicalPerCore;

} }

else else

return 1; return 1;

} }

▲ Show 20 Lines • Show All 183 Lines • Show Last 20 Lines