Conversation
serge-sans-paille
left a comment
serge-sans-paille
left a comment
There was a problem hiding this comment.
Some early review, a few open questions but nothing looks bad here. Way to go!
| { | ||
| inline void get_cpuid(int reg[4], int level, int count = 0) noexcept; | ||
|
|
||
| inline std::uint32_t get_xcr0_low() noexcept; |
There was a problem hiding this comment.
it would be good to somehow ensure that this type is the same as ref_t
There was a problem hiding this comment.
And this could be a private static method for better encapsulation right?
| using reg_t = std::uint32_t; | ||
|
|
||
| /** Parse a XCR0 value into individual components. */ | ||
| constexpr explicit x86_xcr0(reg_t low) noexcept |
There was a problem hiding this comment.
if read is the only way to build this class, this should be private
There was a problem hiding this comment.
I think it's useful to leave at least a default constructor, so that one can create it somewhere and only conditionally read the value etc.
| constexpr bool fma4() const noexcept | ||
| { | ||
| return utils::bit_is_set<16>(m_regs.reg8[2]); | ||
| } |
There was a problem hiding this comment.
Open question: should we have a macro that makes the generation of those field cleaner to the eye?
There was a problem hiding this comment.
Either way. I am personally not big on macros. I find it easier read this way (could also be made to fit on one line) and find&replace is quite enough a change is required.
|
|
||
| namespace detail | ||
| { | ||
| inline void get_cpuid(int reg[4], int level, int count) noexcept |
There was a problem hiding this comment.
Same here: could be private for better encapsulation.
There was a problem hiding this comment.
On the contrary, I was wondering whether to make it part of public API.
It's a well defined function, unlikely to change, and that could be useful for users to get features we do not expose (not sure we'll go to cover 100% of cpuid).
| * The full license is in the file LICENSE, distributed with this software. * | ||
| ****************************************************************************/ | ||
|
|
||
| #include "../config/xsimd_inline.hpp" |
There was a problem hiding this comment.
why do you need this?
There was a problem hiding this comment.
It for header including what they use (otherwise I get nasty errors in my editor).
But this one did not belong here but in xsimd_register.hpp where XSIMD_INLINE is used.
AntoinePrv
left a comment
AntoinePrv
left a comment
There was a problem hiding this comment.
Comments on change may make relative to the previous implementation
| #elif defined(__INTEL_COMPILER) | ||
| __cpuid(reg.data(), level); |
There was a problem hiding this comment.
Should we change to use inline ASM for intel compiler? Missing the count option here.
| #elif defined(_MSC_VER) && _MSC_VER >= 1400 | ||
| return static_cast<xcr0_reg_t>(_xgetbv(0)); | ||
|
|
||
| #elif defined(__GNUC__) |
There was a problem hiding this comment.
What about __clang__ and __INTEL_COMPILER? Should we reproduce the get_cpuid pattern?
|
@serge-sans-paille what do you think of this? I wanted to add an |
2 participants
This is a no-addition, non-breaking refactor of existing CPU id features as first class citizen as part of #1245.
supported_archkeeps the same structure and merges use of both class at the moment but both class could be combined a user-friendlyx86_cpu_features.