-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benchmarks: fix detection of supported codecs on x86 #82
Conversation
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
codec->dec = base64_stream_decode_avx2; | ||
return true; | ||
}; | ||
if( ssse3_supported()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, copy/paste error, should be avx_supported()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the code style dictates a space after if
:
if (avx_supported()) {
But I can fix that in the rebase.
return false; | ||
} | ||
#else | ||
static bool avx2_supported(void) {return false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static bool avx2_supported(void) {return false} | |
static bool avx2_supported(void) {return false;} |
max_level = info[0]; | ||
#else | ||
max_level = __get_cpuid_max(0, NULL); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated and should be split out into a separate function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static inline unsigned int x86_cpuid_max_level (void)
{
#ifdef BASE64_X86_SIMD
#ifdef _MSC_VER
int info[4U];
__cpuidex(info, 0, 0);
return info[0U];
#else
return __get_cpuid_max(0, NULL);
#endif
#else
return 0U;
#endif
}
return false; | ||
} | ||
#else | ||
static bool sse42_supported(void) {return false;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like these tiny vestigial functions. They don't even follow the regular formatting of functions. My suggestion would be something like this instead:
static bool sse42_supported (void)
{
#if HAVE_SSE42
...
#else
return false;
#endif
}
static bool avx_supported(void); | ||
static bool sse42_supported(void); | ||
static bool sse41_supported(void); | ||
static bool ssse3_supported(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These forward declarations can be elided by defining the functions before first use.
bool check; | ||
|
||
check = flags & BASE64_CHECK_SUPPORT; | ||
if (check) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit verbose?
if (flags & BASE64_CHECK_SUPPORT) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also elides the inclusion of <stdbool.h>
.
@@ -53,6 +53,7 @@ extern "C" { | |||
#define BASE64_FORCE_SSE41 (1 << 5) | |||
#define BASE64_FORCE_SSE42 (1 << 6) | |||
#define BASE64_FORCE_AVX (1 << 7) | |||
#define BASE64_CHECK_SUPPORT (1 << 15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag is not documented in README.md
. I also don't understand why the bitmasks make a numerical jump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the flag makes sense. I would expect that even when the user forces a codec, we use runtime feature detection to check whether the codec is actually available. I can't see the use case for wanting to force an AVX2 codec on some low power machine and expecting anything sane to happen. If anything, the sane thing would be to return false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but that is what the comment said (force even if there is no CPU support) and I was not sure if I would be disturbing any test case by changing the default behavior. Then the bitmask is misused (not to add a codec but to change behavior), so I thought to leave room for future codecs.
But from my pov it would be best to not add the bitmask at all. So, if you agree...
Thnaks @htot, this is an interesting pull request. I like where this is going and I think it's a good idea to factor out the x86 feature detection bits. I have some ideas about how to enhance this concept and I may come up with a counter-proposal of my own. |
Sure, I'll wait to see what you come up with before attempting a v2. |
This fixes codec_supported as used by
benchmarks
.benchmarks
forces all codecs built-in at compile time. It does run a test on a test string but this can cause an exception with certain codecs. To this end we factor out cpu capability detection from lib/codec_choose, introduce an additional flag codec flag BASE64_CHECK_SUPPORT, when set this performs cpu capability detection when forced prior to running the test and prevents running the codec if not supported. This ways existing code running the library is not affected.Signed-off-by: Ferry Toth ftoth@exalondelft.nl