|WARNING| net/crc: make NEON CRC folding logic similar to x86 SSE

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170704

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-06

# Review of series.patch

## Summary

This patch optimizes NEON CRC folding logic by aligning it with x86 SSE patterns. The changes include replacing variable-length shift switch cases with table-based shifts, adjusting fold constants to use PMULL/PMULL2 instructions, and streamlining the reduction logic.

## Findings

### Errors

**1. Incorrect logic in partial bytes processing (lines 222-232)**

The partial bytes section builds `b` using `vmvnq_u8()` and `vbslq_u8()` with a mask derived from `vcgezq_s8()` on the shift table indices. However, the shift table contains small unsigned values (0x00-0x0f) which will never be negative when interpreted as signed bytes. This means `vcgezq_s8()` will always produce all-ones (true), causing `vbslq_u8()` to always select `last16` and never select `t2`.

The original code used:
```c
b = vorrq_u64(b, vandq_u64(mask, last16));
```

The new code attempts to blend `last16` and `t2` based on whether the shift table indices are negative, but the shift table values are `0x00-0x0f` (all non-negative).

**Suggested fix:**
```c
t1 = vld1q_u8(crc_neon_shift_tab + rem);
a  = vreinterpretq_u64_u8(vqtbl1q_u8(vreinterpretq_u8_u64(fold), t1));
t2 = vld1q_u8(crc_neon_shift_tab + 16 + rem);
b  = vreinterpretq_u64_u8(vbslq_u8(
	vcgeq_u8(t1, vdupq_n_u8(16)),
	vqtbl1q_u8(last16, vsubq_u8(t1, vdupq_n_u8(16))),
	vqtbl1q_u8(vreinterpretq_u8_u64(fold), t2)));
```

Or alternatively, use the x86 pattern more directly:
```c
uint8x16_t mask1 = vld1q_u8(crc_neon_shift_tab + rem);
uint8x16_t mask2 = vld1q_u8(crc_neon_shift_tab + 16 + rem);
a = vreinterpretq_u64_u8(vqtbl1q_u8(vreinterpretq_u8_u64(fold), mask1));
b = vreinterpretq_u64_u8(vorrq_u8(
	vqtbl1q_u8(vreinterpretq_u8_u64(fold), mask2),
	vqtbl1q_u8(last16, mask1)));
```

**2. Missing error propagation check in init section (line 241)**

The function `rte_net_crc_neon_init()` does not return a value or have any error handling. While not a bug in this patch (it's pre-existing), ensure that the initialization cannot fail silently if the constants are invalid or if the NEON PMULL feature is not available.

### Warnings

**1. Shift table size mismatch (line 27)**

The comment at line 38 states "number of bytes to shift left reg by (0-16)" but the usage at line 203 calls `neon_shift_left(fold, 16 - data_len)`, which when `data_len = 0` would result in `num = 16`. The table access would then be `crc_neon_shift_tab + 16 - 16 = crc_neon_shift_tab + 0`, which is within bounds. However, when `data_len < 4`, line 198 calls `neon_shift_left(fold, 8 - data_len)`, which for `data_len = 0` would be `8`, also within bounds.

The maximum shift is indeed 16 bytes, but the table should be verified to handle all edge cases. The current 32-byte table is correctly sized: indices 0-15 for the shifted-out pattern, indices 16-31 for the shift-in pattern.

**No issue here** - the table size is correct for shifts 0-16.

**2. Partial bytes logic complexity**

The new partial bytes logic (lines 222-232) is more complex than the original. While optimization is the goal, the logic must be verified for correctness across all values of `rem` (0-15). Given the error identified above, this section needs rework and thorough testing.

**3. Missing documentation update**

The commit message states this patch "aligns with x86 SSE logic," but there is no corresponding documentation update in `doc/guides/prog_guide/` or release notes. Since this is an internal optimization without API changes, release notes are not required. However, if this changes performance characteristics significantly, it might warrant a note.

### Info

**1. Fold constant reordering (lines 254-261)**

The reordering of constants in `rte_net_crc_neon_init()` to swap the order within each pair is intentional to support PMULL vs PMULL2 in the folding logic. This is a correct change that matches the algorithm adjustment in `crcr32_folding_round()` where lane 0 and lane 1 access patterns changed (line 74 and 78).

**2. Removal of static mask arrays (lines 126-130 removed)**

The removal of `mask1` and `mask2` static arrays in `crcr32_reduce_64_to_32()` reduces memory footprint and uses `vsetq_lane_u32()` instead, which is a good optimization. The logic change at line 137 removing one `veorq_u64()` operation appears to match the x86 logic, but should be verified against test vectors.

---

## Summary of Required Fixes

1. **Critical:** Fix the partial bytes blending logic (lines 222-232) - the mask construction is incorrect
2. Verify the `crcr32_reduce_64_to_32()` logic change produces correct results (can be validated via existing test suite)

## Testing Recommendations

- Run `app/test/test_crc` to verify correctness
- Test edge cases: `data_len = 0, 1, 2, 3, 4, 15, 16, 17, 31, 32`
- Benchmark performance impact on ARMv8 targets with PMULL