chore(string): Add memcchr function - #3430
stephanmeesters wants to merge 1 commit into
Conversation
This comment was marked as spam.
This comment was marked as spam.
|
| #if defined(_MSC_VER) && _MSC_VER < 1300 | ||
| #include "stringex_memcchr_x86asm.inl" | ||
| #else | ||
| #if defined(_M_IX86) || defined(_M_X64) || defined(__SSE2__) | ||
| #include "stringex_memcchr_sse2.inl" |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes—runtime dispatch is the appropriate fix for 32-bit builds that may run on pre-SSE2 CPUs.
Keep the SSE2 implementation compiled for x86, but expose it under a separate internal name and have memcchr select between it and the portable implementation based on CPUID leaf 1, EDX bit 26. Cache the selected implementation so CPUID is only queried once. The VC6 path should perform the same check before calling the inline-assembly implementation; it must not execute any SSE2 instruction during feature detection.
x64 can continue using SSE2 unconditionally, since SSE2 is mandatory there.
There was a problem hiding this comment.
Chat Gippy says
If the executable is intended to run on any x86-64 CPU
No runtime guard is normally needed. SSE2 is part of the mandatory x86-64 baseline. Every conforming x86-64 CPU supports SSE2.
For example:
#include <emmintrin.h>
__m128i add(__m128i a, __m128i b) {
return _mm_add_epi32(a, b);
}
Compiling this as a normal x86-64 executable is safe:
gcc -O2 -march=x86-64 foo.c -o fooThe resulting program can assume SSE2 when running on an x86-64 processor.
If you also want a 32-bit x86 executable
Then yes, you need to consider the CPU baseline. SSE2 is not guaranteed on all 32-bit x86 processors.
You have two common approaches:
1. Require SSE2
Compile with SSE2 enabled and document that requirement:
gcc -m32 -msse2 -O2 foo.c -o fooThen running it on a CPU without SSE2 is not safe—the process may fault when it executes an SSE2 instruction.
2. Runtime dispatch
If you need to support CPUs both with and without SSE2, keep a non-SSE2 implementation and select it at runtime:
if (cpu_has_sse2()) {
sse2_version();
} else {
scalar_version();
}The important part is that the compiler must not emit SSE2 instructions into the CPU-independent path. Typically you compile the SSE2 implementation separately with the appropriate target options and dispatch to it only after detection.
One important distinction
There are two different things that can happen:
gcc -msse2 ...means “SSE2 instructions are allowed/required in the generated code.”
Whereas:
gcc -march=x86-64-v3 ...means “the target CPU is assumed to support a larger collection of ISA extensions.”
For x86-64, -msse2 by itself doesn't imply AVX, AVX2, etc. SSE2 is already part of the x86-64 baseline.
So, in short:
| Target | SSE2 runtime guard? |
|---|---|
| x86-64 | No — SSE2 is guaranteed |
| 32-bit x86, SSE2 required | No, but unsupported CPUs cannot run it |
| 32-bit x86, broad CPU support | Yes, use runtime dispatch |
| ARM/other architecture | SSE2 code isn't executable there; provide another implementation |
If you're asking because you're compiling with GCC/Clang and using -msse2, -march, or __attribute__((target("sse2"))), I can also explain the subtle cases where the compiler can accidentally put SSE2 instructions into the supposedly generic code path.
There was a problem hiding this comment.
Chat Gippy is right about the x64 case: _M_X64 can select the SSE2 implementation unconditionally because SSE2 is part of the x86-64 baseline. A runtime guard is not needed there.
The concern is specifically the 32-bit paths. _M_IX86 does not mean that the target CPU supports SSE2, so the current condition selects stringex_memcchr_sse2.inl for every 32-bit MSVC build. The VC6 branch is even more direct: it always includes the SSE2 inline-assembly implementation. Those binaries can still run on pre-SSE2 32-bit CPUs, where the first SSE2 instruction will fault.
Thus the fix should be scoped to 32-bit builds: retain unconditional SSE2 for _M_X64, but either document an SSE2 requirement for 32-bit binaries or add runtime dispatch to the portable implementation for 32-bit targets. Builds that explicitly define __SSE2__ already express that requirement and do not need an additional guard.
| #if defined(_MSC_VER) && _MSC_VER < 1300 | ||
| #include "stringex_memcchr_x86asm.inl" | ||
| #else | ||
| #if defined(_M_IX86) || defined(_M_X64) || defined(__SSE2__) | ||
| #include "stringex_memcchr_sse2.inl" | ||
| #else | ||
| #include "stringex_memcchr.inl" | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
memcchr has three separate versions, including hand-encoded assembly, but this change adds no tests for their results or length boundaries. Shared tests for the first mismatch, all-equal input, and lengths around each block size would catch differences between builds.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| while (n >= 8) { | ||
| uint64_t v; | ||
| memcpy(&v, p, sizeof(v)); | ||
| if (v != repeated64) break; |
There was a problem hiding this comment.
The new if puts break on the condition's line. The repository requires statement bodies on separate lines so developers can set a breakpoint on the action. The SSE2 version also uses this pattern. Put these bodies on their own lines before merging.
Rule Used: Always place if/else/for/while statement bodies on separate lines from the condition to allow precise debugger breakpoint placement. Prefer the format: cpp if (condition) doSomethingNextLine(); over: ```cpp if (condition) doSomethingOnSam... (source)
Learned From
TheSuperHackers/GeneralsGameCode#2067
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ++p; | ||
| --n; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
The portable version returns 0 as a null pointer. The repository requires nullptr for null pointer literals in C++, as the SSE2 version already uses. Change this return before merging.
| return 0; | |
| return nullptr; |
Rule Used: Use nullptr instead of NULL for null pointer literals in C++ code (source)
Learned From
TheSuperHackers/GeneralsGameCode#2067
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This adds the
memcchrfunction, which finds the first byte that differs from a given value.Provided are three versions: SSE2 (for win32 preset), VC6 inline assembly (also uses SSE2), and a portable version.
AI was used. Checked for correctness between versions and reference versions.