Reduce latency cycles for _mm_movemask_epi8 on Armv7-A - #769
Conversation
Reduce latency cycles for '_mm_movemask_epi8' on Armv7-A. In brief, for current VSRA implementation, each VSRA requires 5 cycles, leading to at most 30 cycles for the whole conversion. This commit utilizes VPADD, with at most 3 cycles, to reduce latency cycles to at most 21 cycles. Though it requires a Q-register load, which will become a burden when the conversion calling for many times, this commit still wins on latency cycles. As I don't have any physical Armv7-A platforms by my side, it will be a great appreciation if any folks can assist on benchmarking.
Performance Tier Documentation ReminderThe This is not an error - just a reminder to update the documentation if your changes affect intrinsic implementations. To regenerate the performance tier report: python3 scripts/gen-perf-report.py --clang-ast --weighted > perf-tier.mdFor detailed analysis: python3 scripts/analyze-tiers.py --clang-ast --weighted --markdown |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:210">
P3: The Tier 3 list documents `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function` as intrinsics, but these are internal helper names in sse2neon.h, not the public API. The real intrinsics are `_mm_shufflehi_epi16` / `_mm_shufflelo_epi16`, which are what users and tests call. Rename the two entries so the tier list references the actual intrinsic names and doesn't invent nonexistent ones.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| `_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16` | ||
| `_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps` | ||
| `_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8` | ||
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` |
There was a problem hiding this comment.
P3: The Tier 3 list documents _mm_shufflehi_epi16_function and _mm_shufflelo_epi16_function as intrinsics, but these are internal helper names in sse2neon.h, not the public API. The real intrinsics are _mm_shufflehi_epi16 / _mm_shufflelo_epi16, which are what users and tests call. Rename the two entries so the tier list references the actual intrinsic names and doesn't invent nonexistent ones.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 210:
<comment>The Tier 3 list documents `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function` as intrinsics, but these are internal helper names in sse2neon.h, not the public API. The real intrinsics are `_mm_shufflehi_epi16` / `_mm_shufflelo_epi16`, which are what users and tests call. Rename the two entries so the tier list references the actual intrinsic names and doesn't invent nonexistent ones.</comment>
<file context>
@@ -61,167 +61,173 @@ algorithms when porting performance-critical code.
+`_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16`
+`_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps`
+`_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8`
+`_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function`
+`_mm_sign_epi16`, `_mm_sign_epi32`, `_mm_sign_epi8`, `_mm_sign_pi16`
+`_mm_sign_pi32`, `_mm_sign_pi8`, `_mm_srai_epi32`, `_mm_test_mix_ones_zeros`
</file context>
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` | |
| `_mm_shufflehi_epi16`, `_mm_shufflelo_epi16` |
_mm_movemask_epi8 on Armv7-A_mm_movemask_epi8 on Armv7-A
Update this by running the script. |
GitHub Actions provides standard GitHub-hosted runners for Ubuntu on Arm64, capable of running Arm32 executables. |
Thanks for the reminder! |
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/bench_movemask.cpp">
<violation number="1" location="tests/bench_movemask.cpp:66">
P2: In bench_throughput, the accumulator `acc` is `int` but is added to 10M times; for the all-ones input each movemask returns 0xFFFF so the running total far exceeds INT_MAX. That is signed-overflow UB in C++, which can let the optimizer reorder or elide parts of the timed loop and invalidate the throughput measurement you're trying to produce. Use `int64_t` (or a `uint32_t`) accumulator.</violation>
</file>
<file name=".github/workflows/benchmark-a32.yml">
<violation number="1" location=".github/workflows/benchmark-a32.yml:3">
P2: When this benchmark is triggered by a pull request (via `pull_request_target`), `actions/checkout` without an explicit `ref` checks out the base branch, so the PR is not actually benchmarked — the job runs against the repository's existing `sse2neon.h`, defeating the purpose of validating the PR's `_mm_movemask_epi8` change. It also uses the `pull_request_target` event, which executes in the base repository's trusted context on untrusted PR payloads; the commit-message guard here is weak and bypassable, and no untrusted code is executed in a sandbox. Consider running on `pull_request` instead, or keeping `pull_request_target` but checking out `ref: ${{ github.event.pull_request.head.sha }}` (and treating the job as untrusted) so the benchmark measures the PR's code.</violation>
<violation number="2" location=".github/workflows/benchmark-a32.yml:10">
P2: Reported latency will describe the A64 host's A32 execution path, not Armv7-A, so it cannot validate the stated Armv7-A cycle reduction. Run this performance benchmark on an Armv7-A runner/device, or label results as A64 A32-compat measurements.</violation>
</file>
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:37">
P2: The per-tier classification numbers introduced here are not reproducible from the documented regeneration command. The file header says to regenerate with `python3 scripts/gen-perf-report.py > perf-tier.md`, but running exactly that against the current `sse2neon.h` yields Tier 1(264)/Tier 2(108)/Tier 3(60)/Tier 4(53) (Total 485), while this commit records Tier 1(314)/Tier 2(73)/Tier 3(46)/Tier 4(52) (also Total 485). The total is right but the tier distribution (and therefore the summary percentages 64.7%/24.5%/10.7%) disagrees with the authoritative generator, so the doc's classification of individual intrinsics is likely stale or produced by an undocumented analysis mode. Recommend regenerating perf-tier.md with the documented command and committing the matching counts so the reference doc stays accurate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| int acc = 0; | ||
| uint64_t t0 = now_ns(); | ||
| for (int64_t i = 0; i < iters; i++) { | ||
| acc += _mm_movemask_epi8(data[i & (n_data - 1)]); |
There was a problem hiding this comment.
P2: In bench_throughput, the accumulator acc is int but is added to 10M times; for the all-ones input each movemask returns 0xFFFF so the running total far exceeds INT_MAX. That is signed-overflow UB in C++, which can let the optimizer reorder or elide parts of the timed loop and invalidate the throughput measurement you're trying to produce. Use int64_t (or a uint32_t) accumulator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/bench_movemask.cpp, line 66:
<comment>In bench_throughput, the accumulator `acc` is `int` but is added to 10M times; for the all-ones input each movemask returns 0xFFFF so the running total far exceeds INT_MAX. That is signed-overflow UB in C++, which can let the optimizer reorder or elide parts of the timed loop and invalidate the throughput measurement you're trying to produce. Use `int64_t` (or a `uint32_t`) accumulator.</comment>
<file context>
@@ -0,0 +1,231 @@
+ int acc = 0;
+ uint64_t t0 = now_ns();
+ for (int64_t i = 0; i < iters; i++) {
+ acc += _mm_movemask_epi8(data[i & (n_data - 1)]);
+ }
+ uint64_t t1 = now_ns();
</file context>
| name: Benchmark A32 (Armv7-A) on A64 Native | ||
| if: contains(toJSON(github.event.head_commit.message), 'Merge pull request ') == false | ||
| timeout-minutes: 30 | ||
| runs-on: ubuntu-24.04-arm |
There was a problem hiding this comment.
P2: Reported latency will describe the A64 host's A32 execution path, not Armv7-A, so it cannot validate the stated Armv7-A cycle reduction. Run this performance benchmark on an Armv7-A runner/device, or label results as A64 A32-compat measurements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/benchmark-a32.yml, line 10:
<comment>Reported latency will describe the A64 host's A32 execution path, not Armv7-A, so it cannot validate the stated Armv7-A cycle reduction. Run this performance benchmark on an Armv7-A runner/device, or label results as A64 A32-compat measurements.</comment>
<file context>
@@ -0,0 +1,25 @@
+ name: Benchmark A32 (Armv7-A) on A64 Native
+ if: contains(toJSON(github.event.head_commit.message), 'Merge pull request ') == false
+ timeout-minutes: 30
+ runs-on: ubuntu-24.04-arm
+ steps:
+ - name: Checkout code
</file context>
| | Complex Emulation (T4) | 14 (3.0%) | | ||
| | Avg NEON Ops/Intrinsic | 1.89 | | ||
| | Total SSE Intrinsics | 485 | | ||
| | Direct Mappings (T1) | 314 (64.7%) | |
There was a problem hiding this comment.
P2: The per-tier classification numbers introduced here are not reproducible from the documented regeneration command. The file header says to regenerate with python3 scripts/gen-perf-report.py > perf-tier.md, but running exactly that against the current sse2neon.h yields Tier 1(264)/Tier 2(108)/Tier 3(60)/Tier 4(53) (Total 485), while this commit records Tier 1(314)/Tier 2(73)/Tier 3(46)/Tier 4(52) (also Total 485). The total is right but the tier distribution (and therefore the summary percentages 64.7%/24.5%/10.7%) disagrees with the authoritative generator, so the doc's classification of individual intrinsics is likely stale or produced by an undocumented analysis mode. Recommend regenerating perf-tier.md with the documented command and committing the matching counts so the reference doc stays accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 37:
<comment>The per-tier classification numbers introduced here are not reproducible from the documented regeneration command. The file header says to regenerate with `python3 scripts/gen-perf-report.py > perf-tier.md`, but running exactly that against the current `sse2neon.h` yields Tier 1(264)/Tier 2(108)/Tier 3(60)/Tier 4(53) (Total 485), while this commit records Tier 1(314)/Tier 2(73)/Tier 3(46)/Tier 4(52) (also Total 485). The total is right but the tier distribution (and therefore the summary percentages 64.7%/24.5%/10.7%) disagrees with the authoritative generator, so the doc's classification of individual intrinsics is likely stale or produced by an undocumented analysis mode. Recommend regenerating perf-tier.md with the documented command and committing the matching counts so the reference doc stays accurate.</comment>
<file context>
@@ -33,11 +33,11 @@ Cycle estimates based on ARM Cortex-A72 (ARMv8-A) Software Optimization Guide.
-| Complex Emulation (T4) | 14 (3.0%) |
-| Avg NEON Ops/Intrinsic | 1.89 |
+| Total SSE Intrinsics | 485 |
+| Direct Mappings (T1) | 314 (64.7%) |
+| Moderate Emulation (T2-T3) | 119 (24.5%) |
+| Complex Emulation (T4) | 52 (10.7%) |
</file context>
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:210">
P3: The new Tier 3 entries list internal helper names `_mm_shufflehi_epi16_function`/`_mm_shufflelo_epi16_function` instead of the public SSE intrinsics `_mm_shufflehi_epi16`/`_mm_shufflelo_epi16`. Readers of the perf-tier doc won't find these `_function` names in the header's public API or in Intel's intrinsic guide, which makes the classification harder to act on; use the public intrinsic names for consistency with the rest of the table.</violation>
</file>
<file name=".github/workflows/benchmark-a32.yml">
<violation number="1" location=".github/workflows/benchmark-a32.yml:3">
P1: PR benchmark runs measure the target branch rather than the proposed movemask implementation, so results cannot validate or compare this PR. Use `pull_request` for this unprivileged build/run workflow (or explicitly arrange a safe PR-code checkout).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| `_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16` | ||
| `_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps` | ||
| `_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8` | ||
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` |
There was a problem hiding this comment.
P3: The new Tier 3 entries list internal helper names _mm_shufflehi_epi16_function/_mm_shufflelo_epi16_function instead of the public SSE intrinsics _mm_shufflehi_epi16/_mm_shufflelo_epi16. Readers of the perf-tier doc won't find these _function names in the header's public API or in Intel's intrinsic guide, which makes the classification harder to act on; use the public intrinsic names for consistency with the rest of the table.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 210:
<comment>The new Tier 3 entries list internal helper names `_mm_shufflehi_epi16_function`/`_mm_shufflelo_epi16_function` instead of the public SSE intrinsics `_mm_shufflehi_epi16`/`_mm_shufflelo_epi16`. Readers of the perf-tier doc won't find these `_function` names in the header's public API or in Intel's intrinsic guide, which makes the classification harder to act on; use the public intrinsic names for consistency with the rest of the table.</comment>
<file context>
@@ -61,167 +61,173 @@ algorithms when porting performance-critical code.
+`_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16`
+`_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps`
+`_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8`
+`_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function`
+`_mm_sign_epi16`, `_mm_sign_epi32`, `_mm_sign_epi8`, `_mm_sign_pi16`
+`_mm_sign_pi32`, `_mm_sign_pi8`, `_mm_srai_epi32`, `_mm_test_mix_ones_zeros`
</file context>
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` | |
| `_mm_shufflehi_epi16`, `_mm_shufflelo_epi16` |
Consider to send pull request(s) to enable continuous benchmarking for CI/CD. |
There was a problem hiding this comment.
5 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:210">
P3: The Tier 3 list references `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function`, but those are internal helper functions in sse2neon.h rather than public SSE intrinsics. Readers looking these up won't find them as exposed API; the entries should be the actual intrinsics `_mm_shufflehi_epi16` and `_mm_shufflelo_epi16` (which take an imm constant).</violation>
</file>
<file name="tests/bench_movemask.cpp">
<violation number="1" location="tests/bench_movemask.cpp:66">
P3: The throughput case measures more than movemask throughput: every iteration feeds its result into a single serial `acc +=` chain (and the next ADD must wait on the previous), so on the in-order ARMv7 target the loop is partly serialized by the accumulator rather than exercising true independent-call pipelining. Consider breaking the loop-level dependency, e.g. sum into multiple independent accumulators (unroll by 4) or collect results and reduce after timing, so the reported ns/op reflects the movemask execution-path throughput the change is meant to measure.</violation>
<violation number="2" location="tests/bench_movemask.cpp:82">
P3: The latency loop feeds the movemask through `_mm_set1_epi8((char)(mask & 0xFF))`, so the measured per-iteration latency also includes the `mask & 0xFF` truncation and the NEON vector-broadcast (duplicate) latency on top of `_mm_movemask_epi8`. That extra fixed setup inflates the absolute number, though it stays constant across the old/new implementations so relative comparisons remain valid; worth noting so the ~21-cycle claim isn't read as pure movemask latency.</violation>
</file>
<file name=".github/workflows/benchmark-a32.yml">
<violation number="1" location=".github/workflows/benchmark-a32.yml:7">
P2: This benchmark runs A32 instructions on an AArch64 core in compatibility mode, so the resulting cycle counts reflect Ampere Altra's ARMv7 execution rather than real ARMv7-A silicon. The latency numbers here are a sanity check and cannot validate the microarchitecture-specific latency-reduction claim in the PR description; the on-hardware ARMv7-A benchmarking mentioned there remains necessary before relying on the ~30→21 cycle figures.</violation>
<violation number="2" location=".github/workflows/benchmark-a32.yml:25">
P2: The steps run a cross-compiled ARMv7 armhf binary natively on the AArch64 `ubuntu-24.04-arm` runner by clearing `EXEC_WRAPPER`, which relies on the host kernel supporting 32-bit A32 execution (CONFIG_COMPAT). If the runner kernel lacks this, the `bench-movemask` steps fail with an exec-format error; this is not guaranteed on GitHub-hosted arm64 runners. Consider verifying on the actual runner or keeping the `qemu-arm` fallback for robustness.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| jobs: | ||
| benchmark-a32: | ||
| name: Benchmark A32 (Armv7-A) on A64 Native |
There was a problem hiding this comment.
P2: This benchmark runs A32 instructions on an AArch64 core in compatibility mode, so the resulting cycle counts reflect Ampere Altra's ARMv7 execution rather than real ARMv7-A silicon. The latency numbers here are a sanity check and cannot validate the microarchitecture-specific latency-reduction claim in the PR description; the on-hardware ARMv7-A benchmarking mentioned there remains necessary before relying on the ~30→21 cycle figures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/benchmark-a32.yml, line 7:
<comment>This benchmark runs A32 instructions on an AArch64 core in compatibility mode, so the resulting cycle counts reflect Ampere Altra's ARMv7 execution rather than real ARMv7-A silicon. The latency numbers here are a sanity check and cannot validate the microarchitecture-specific latency-reduction claim in the PR description; the on-hardware ARMv7-A benchmarking mentioned there remains necessary before relying on the ~30→21 cycle figures.</comment>
<file context>
@@ -0,0 +1,29 @@
+
+jobs:
+ benchmark-a32:
+ name: Benchmark A32 (Armv7-A) on A64 Native
+ if: contains(toJSON(github.event.head_commit.message), 'Merge pull request ') == false
+ timeout-minutes: 30
</file context>
| run: | | ||
| echo "=== Optimized Implementation (VPADD) ===" | ||
| make clean > /dev/null | ||
| make bench-movemask CROSS_COMPILE=arm-linux-gnueabihf- EXEC_WRAPPER= |
There was a problem hiding this comment.
P2: The steps run a cross-compiled ARMv7 armhf binary natively on the AArch64 ubuntu-24.04-arm runner by clearing EXEC_WRAPPER, which relies on the host kernel supporting 32-bit A32 execution (CONFIG_COMPAT). If the runner kernel lacks this, the bench-movemask steps fail with an exec-format error; this is not guaranteed on GitHub-hosted arm64 runners. Consider verifying on the actual runner or keeping the qemu-arm fallback for robustness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/benchmark-a32.yml, line 25:
<comment>The steps run a cross-compiled ARMv7 armhf binary natively on the AArch64 `ubuntu-24.04-arm` runner by clearing `EXEC_WRAPPER`, which relies on the host kernel supporting 32-bit A32 execution (CONFIG_COMPAT). If the runner kernel lacks this, the `bench-movemask` steps fail with an exec-format error; this is not guaranteed on GitHub-hosted arm64 runners. Consider verifying on the actual runner or keeping the `qemu-arm` fallback for robustness.</comment>
<file context>
@@ -0,0 +1,29 @@
+ run: |
+ echo "=== Optimized Implementation (VPADD) ==="
+ make clean > /dev/null
+ make bench-movemask CROSS_COMPILE=arm-linux-gnueabihf- EXEC_WRAPPER=
+
+ echo "=== Original Implementation (VSRA Baseline) ==="
</file context>
| `_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16` | ||
| `_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps` | ||
| `_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8` | ||
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` |
There was a problem hiding this comment.
P3: The Tier 3 list references _mm_shufflehi_epi16_function and _mm_shufflelo_epi16_function, but those are internal helper functions in sse2neon.h rather than public SSE intrinsics. Readers looking these up won't find them as exposed API; the entries should be the actual intrinsics _mm_shufflehi_epi16 and _mm_shufflelo_epi16 (which take an imm constant).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 210:
<comment>The Tier 3 list references `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function`, but those are internal helper functions in sse2neon.h rather than public SSE intrinsics. Readers looking these up won't find them as exposed API; the entries should be the actual intrinsics `_mm_shufflehi_epi16` and `_mm_shufflelo_epi16` (which take an imm constant).</comment>
<file context>
@@ -61,167 +61,173 @@ algorithms when porting performance-critical code.
+`_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16`
+`_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps`
+`_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8`
+`_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function`
+`_mm_sign_epi16`, `_mm_sign_epi32`, `_mm_sign_epi8`, `_mm_sign_pi16`
+`_mm_sign_pi32`, `_mm_sign_pi8`, `_mm_srai_epi32`, `_mm_test_mix_ones_zeros`
</file context>
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` | |
| `_mm_shufflehi_epi16`, `_mm_shufflelo_epi16` |
| __m128i vec = _mm_set1_epi8((char) 0xA5); | ||
| uint64_t t0 = now_ns(); | ||
| for (int64_t i = 0; i < iters; i++) { | ||
| int mask = _mm_movemask_epi8(vec); |
There was a problem hiding this comment.
P3: The latency loop feeds the movemask through _mm_set1_epi8((char)(mask & 0xFF)), so the measured per-iteration latency also includes the mask & 0xFF truncation and the NEON vector-broadcast (duplicate) latency on top of _mm_movemask_epi8. That extra fixed setup inflates the absolute number, though it stays constant across the old/new implementations so relative comparisons remain valid; worth noting so the ~21-cycle claim isn't read as pure movemask latency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/bench_movemask.cpp, line 82:
<comment>The latency loop feeds the movemask through `_mm_set1_epi8((char)(mask & 0xFF))`, so the measured per-iteration latency also includes the `mask & 0xFF` truncation and the NEON vector-broadcast (duplicate) latency on top of `_mm_movemask_epi8`. That extra fixed setup inflates the absolute number, though it stays constant across the old/new implementations so relative comparisons remain valid; worth noting so the ~21-cycle claim isn't read as pure movemask latency.</comment>
<file context>
@@ -0,0 +1,231 @@
+ __m128i vec = _mm_set1_epi8((char) 0xA5);
+ uint64_t t0 = now_ns();
+ for (int64_t i = 0; i < iters; i++) {
+ int mask = _mm_movemask_epi8(vec);
+ /* Feed the result back as input to create a true data dependency. */
+ vec = _mm_set1_epi8((char) (mask & 0xFF));
</file context>
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/bench_movemask.cpp">
<violation number="1" location="tests/bench_movemask.cpp:149">
P2: The first four throughput datasets are filled with compile-time constants (setzero, set1 0xFF, constant set_epi8 patterns), so at -O3 the inlined _mm_movemask_epi8 loop in bench_throughput constant-folds, and All-zero/All-ones/Alternating/Cmp-result will report ~0 ns instead of real throughput. Only the xorshift-derived Random case reflects actual hardware. Populate these arrays from runtime (non-constant) data so the measurement isn't folded away.</violation>
</file>
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:65">
P3: The new table shows _mm_maddubs_epi16 at 22 NEON Ops, while README.md documents the same intrinsic as using 13 instructions. If 'NEON Ops' counts differently than instructions, please say so; otherwise reconcile the two figures so the perf tables stay a trustworthy single source.</violation>
</file>
<file name=".github/workflows/benchmark-a32.yml">
<violation number="1" location=".github/workflows/benchmark-a32.yml:10">
P3: The goal of this PR is a ~9-cycle latency reduction validated by microbenchmark, but the numbers come from a shared, frequency-scaling arm64 VM where nanosecond-level latency/dependent-chain measurements are noisy and hard to reproduce. The results are indicative only and could easily hide or falsely confirm the expected gain. Consider documenting the expected variance, running more repetitions, or validating on pinned/dedicated ARMv7 hardware before treating the gain as confirmed.</violation>
</file>
<file name="sse2neon.h">
<violation number="1" location="sse2neon.h:5777">
P3: This patch embeds a benchmark-only code fork inside the shipped library header. The `BENCH_ORIGINAL_MOVEMASK_EPI8` macro is defined nowhere in sse2neon.h (it is only supplied by the Makefile/CI via `CXXFLAGS="-DBENCH_ORIGINAL_MOVEMASK_EPI8"` to compare the VSRA baseline for the benchmark). As a result, the production header now carries a full alternate ARMv7 implementation that is dead in every real build, and the compile-time behavior of `_mm_movemask_epi8` silently changes depending on an undocumented, test-only macro a downstream consumer might define. Consider keeping the baseline out of the production header (e.g., benchmark against a fixed reference implementation in tests/bench_movemask.cpp, or drop the hook once the benchmarking is done) so the library ships a single default path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| __m128i data_rand[N_DATA]; | ||
|
|
||
| for (int i = 0; i < N_DATA; i++) { | ||
| data_zero[i] = _mm_setzero_si128(); |
There was a problem hiding this comment.
P2: The first four throughput datasets are filled with compile-time constants (setzero, set1 0xFF, constant set_epi8 patterns), so at -O3 the inlined _mm_movemask_epi8 loop in bench_throughput constant-folds, and All-zero/All-ones/Alternating/Cmp-result will report ~0 ns instead of real throughput. Only the xorshift-derived Random case reflects actual hardware. Populate these arrays from runtime (non-constant) data so the measurement isn't folded away.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/bench_movemask.cpp, line 149:
<comment>The first four throughput datasets are filled with compile-time constants (setzero, set1 0xFF, constant set_epi8 patterns), so at -O3 the inlined _mm_movemask_epi8 loop in bench_throughput constant-folds, and All-zero/All-ones/Alternating/Cmp-result will report ~0 ns instead of real throughput. Only the xorshift-derived Random case reflects actual hardware. Populate these arrays from runtime (non-constant) data so the measurement isn't folded away.</comment>
<file context>
@@ -0,0 +1,231 @@
+ __m128i data_rand[N_DATA];
+
+ for (int i = 0; i < N_DATA; i++) {
+ data_zero[i] = _mm_setzero_si128();
+ data_all[i] = _mm_set1_epi8((char) 0xFF);
+ data_alt[i] = _mm_set_epi8(
</file context>
| |-----------|----------|-------| | ||
| | `_mm_mpsadbw_epu8` | 22 | SAD computation, very expensive | | ||
| | `_mm_mpsadbw_epu8` | 37 | SAD computation, very expensive | | ||
| | `_mm_maddubs_epi16` | 22 | Multiply-add with widening | |
There was a problem hiding this comment.
P3: The new table shows _mm_maddubs_epi16 at 22 NEON Ops, while README.md documents the same intrinsic as using 13 instructions. If 'NEON Ops' counts differently than instructions, please say so; otherwise reconcile the two figures so the perf tables stay a trustworthy single source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 65:
<comment>The new table shows _mm_maddubs_epi16 at 22 NEON Ops, while README.md documents the same intrinsic as using 13 instructions. If 'NEON Ops' counts differently than instructions, please say so; otherwise reconcile the two figures so the perf tables stay a trustworthy single source.</comment>
<file context>
@@ -61,167 +61,173 @@ algorithms when porting performance-critical code.
|-----------|----------|-------|
-| `_mm_mpsadbw_epu8` | 22 | SAD computation, very expensive |
+| `_mm_mpsadbw_epu8` | 37 | SAD computation, very expensive |
+| `_mm_maddubs_epi16` | 22 | Multiply-add with widening |
+| `_mm_round_ps` | 22 | Rounding modes emulation |
+| `_mm_aesenclast_si128` | 20 | Use HW crypto when available |
</file context>
| name: Benchmark A32 (Armv7-A) on A64 Native | ||
| if: contains(toJSON(github.event.head_commit.message), 'Merge pull request ') == false | ||
| timeout-minutes: 30 | ||
| runs-on: ubuntu-24.04-arm |
There was a problem hiding this comment.
P3: The goal of this PR is a ~9-cycle latency reduction validated by microbenchmark, but the numbers come from a shared, frequency-scaling arm64 VM where nanosecond-level latency/dependent-chain measurements are noisy and hard to reproduce. The results are indicative only and could easily hide or falsely confirm the expected gain. Consider documenting the expected variance, running more repetitions, or validating on pinned/dedicated ARMv7 hardware before treating the gain as confirmed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/benchmark-a32.yml, line 10:
<comment>The goal of this PR is a ~9-cycle latency reduction validated by microbenchmark, but the numbers come from a shared, frequency-scaling arm64 VM where nanosecond-level latency/dependent-chain measurements are noisy and hard to reproduce. The results are indicative only and could easily hide or falsely confirm the expected gain. Consider documenting the expected variance, running more repetitions, or validating on pinned/dedicated ARMv7 hardware before treating the gain as confirmed.</comment>
<file context>
@@ -0,0 +1,29 @@
+ name: Benchmark A32 (Armv7-A) on A64 Native
+ if: contains(toJSON(github.event.head_commit.message), 'Merge pull request ') == false
+ timeout-minutes: 30
+ runs-on: ubuntu-24.04-arm
+ steps:
+ - name: Checkout code
</file context>
| // ARMv7: Shift-right-accumulate (no vaddv). | ||
| // | ||
| // Step 1: Extract MSB of each byte | ||
| #if defined(BENCH_ORIGINAL_MOVEMASK_EPI8) |
There was a problem hiding this comment.
P3: This patch embeds a benchmark-only code fork inside the shipped library header. The BENCH_ORIGINAL_MOVEMASK_EPI8 macro is defined nowhere in sse2neon.h (it is only supplied by the Makefile/CI via CXXFLAGS="-DBENCH_ORIGINAL_MOVEMASK_EPI8" to compare the VSRA baseline for the benchmark). As a result, the production header now carries a full alternate ARMv7 implementation that is dead in every real build, and the compile-time behavior of _mm_movemask_epi8 silently changes depending on an undocumented, test-only macro a downstream consumer might define. Consider keeping the baseline out of the production header (e.g., benchmark against a fixed reference implementation in tests/bench_movemask.cpp, or drop the hook once the benchmarking is done) so the library ships a single default path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sse2neon.h, line 5777:
<comment>This patch embeds a benchmark-only code fork inside the shipped library header. The `BENCH_ORIGINAL_MOVEMASK_EPI8` macro is defined nowhere in sse2neon.h (it is only supplied by the Makefile/CI via `CXXFLAGS="-DBENCH_ORIGINAL_MOVEMASK_EPI8"` to compare the VSRA baseline for the benchmark). As a result, the production header now carries a full alternate ARMv7 implementation that is dead in every real build, and the compile-time behavior of `_mm_movemask_epi8` silently changes depending on an undocumented, test-only macro a downstream consumer might define. Consider keeping the baseline out of the production header (e.g., benchmark against a fixed reference implementation in tests/bench_movemask.cpp, or drop the hook once the benchmarking is done) so the library ships a single default path.</comment>
<file context>
@@ -5774,33 +5774,31 @@ FORCE_INLINE int _mm_movemask_epi8(__m128i a)
- // ARMv7: Shift-right-accumulate (no vaddv).
- //
- // Step 1: Extract MSB of each byte
+#if defined(BENCH_ORIGINAL_MOVEMASK_EPI8)
+ // ARMv7: Shift-right-accumulate (baseline)
uint8x16_t msbs = vshrq_n_u8(input, 7);
</file context>
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/benchmark-a32.yml">
<violation number="1" location=".github/workflows/benchmark-a32.yml:25">
P2: The job builds an ARMv7 (armhf) binary and runs it natively via EXEC_WRAPPER= on an AArch64 runner. Native AArch32 execution only works if the runner CPU implements the AArch32 state at EL0 (e.g. Cortex-A / Ampere Altra) and the kernel has CONFIG_COMPAT; Neoverse V-class and similar cores drop AArch32, in which case the binary fails with 'Exec format error'/SIGILL and the benchmark never runs. The repo already uses qemu-arm for ARMv7 in main.yml for exactly this reason. Consider verifying the runner's AArch32 support or falling back to $(EXEC_WRAPPER) (QEMU) as insurance so the job reports numbers rather than dying.</violation>
</file>
<file name="perf-tier.md">
<violation number="1" location="perf-tier.md:210">
P2: The Tier 3 list documents `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function`, but those are internal sse2neon helper functions, not public SSE intrinsics. Users call `_mm_shufflehi_epi16`/`_mm_shufflelo_epi16`, which never appear anywhere in the tier lists, so the perf-tier reference won't match the real, documented API names. Please rename the two entries to the public intrinsic names.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| run: | | ||
| echo "=== Optimized Implementation (VPADD) ===" | ||
| make clean > /dev/null | ||
| make bench-movemask CROSS_COMPILE=arm-linux-gnueabihf- EXEC_WRAPPER= |
There was a problem hiding this comment.
P2: The job builds an ARMv7 (armhf) binary and runs it natively via EXEC_WRAPPER= on an AArch64 runner. Native AArch32 execution only works if the runner CPU implements the AArch32 state at EL0 (e.g. Cortex-A / Ampere Altra) and the kernel has CONFIG_COMPAT; Neoverse V-class and similar cores drop AArch32, in which case the binary fails with 'Exec format error'/SIGILL and the benchmark never runs. The repo already uses qemu-arm for ARMv7 in main.yml for exactly this reason. Consider verifying the runner's AArch32 support or falling back to $(EXEC_WRAPPER) (QEMU) as insurance so the job reports numbers rather than dying.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/benchmark-a32.yml, line 25:
<comment>The job builds an ARMv7 (armhf) binary and runs it natively via EXEC_WRAPPER= on an AArch64 runner. Native AArch32 execution only works if the runner CPU implements the AArch32 state at EL0 (e.g. Cortex-A / Ampere Altra) and the kernel has CONFIG_COMPAT; Neoverse V-class and similar cores drop AArch32, in which case the binary fails with 'Exec format error'/SIGILL and the benchmark never runs. The repo already uses qemu-arm for ARMv7 in main.yml for exactly this reason. Consider verifying the runner's AArch32 support or falling back to $(EXEC_WRAPPER) (QEMU) as insurance so the job reports numbers rather than dying.</comment>
<file context>
@@ -0,0 +1,29 @@
+ run: |
+ echo "=== Optimized Implementation (VPADD) ==="
+ make clean > /dev/null
+ make bench-movemask CROSS_COMPILE=arm-linux-gnueabihf- EXEC_WRAPPER=
+
+ echo "=== Original Implementation (VSRA Baseline) ==="
</file context>
| `_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16` | ||
| `_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps` | ||
| `_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8` | ||
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` |
There was a problem hiding this comment.
P2: The Tier 3 list documents _mm_shufflehi_epi16_function and _mm_shufflelo_epi16_function, but those are internal sse2neon helper functions, not public SSE intrinsics. Users call _mm_shufflehi_epi16/_mm_shufflelo_epi16, which never appear anywhere in the tier lists, so the perf-tier reference won't match the real, documented API names. Please rename the two entries to the public intrinsic names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At perf-tier.md, line 210:
<comment>The Tier 3 list documents `_mm_shufflehi_epi16_function` and `_mm_shufflelo_epi16_function`, but those are internal sse2neon helper functions, not public SSE intrinsics. Users call `_mm_shufflehi_epi16`/`_mm_shufflelo_epi16`, which never appear anywhere in the tier lists, so the perf-tier reference won't match the real, documented API names. Please rename the two entries to the public intrinsic names.</comment>
<file context>
@@ -61,167 +61,173 @@ algorithms when porting performance-critical code.
+`_mm_hsub_pi16`, `_mm_hsub_pi32`, `_mm_hsub_ps`, `_mm_hsubs_epi16`
+`_mm_hsubs_pi16`, `_mm_maddubs_pi16`, `_mm_movehdup_ps`, `_mm_moveldup_ps`
+`_mm_popcnt_u32`, `_mm_popcnt_u64`, `_mm_rcp_ps`, `_mm_sad_pu8`
+`_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function`
+`_mm_sign_epi16`, `_mm_sign_epi32`, `_mm_sign_epi8`, `_mm_sign_pi16`
+`_mm_sign_pi32`, `_mm_sign_pi8`, `_mm_srai_epi32`, `_mm_test_mix_ones_zeros`
</file context>
| `_mm_shufflehi_epi16_function`, `_mm_shufflelo_epi16_function` | |
| `_mm_shufflehi_epi16`, `_mm_shufflelo_epi16` |
Reduce latency cycles for
_mm_movemask_epi8on Armv7-A.In brief, for current VSRA implementation, each VSRA requires 5 cycles, leading to at most 30 cycles for the whole conversion. This commit utilizes VPADD, with at most 3 cycles, to reduce latency cycles to at most 21 cycles.
Though it requires a Q-register load, which will become a burden when the conversion calling for many times, this commit still wins on latency cycles.
As I don't have any physical Armv7-A platforms by my side, it will be a great appreciation if any folks can assist on benchmarking.
You can find the a direct comparison here: https://godbolt.org/z/c4qKrba8o
Original
Proposed
Benchmarking
code