|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

# DPDK Patch Review: net/crc NEON CRC folding logic

## Summary
This patch refactors the NEON CRC implementation to align with x86 SSE patterns. The changes include replacing variable-length shift switch cases with table-based lookups, adjusting fold constants for PMULL/PMULL2, and simplifying the reduction logic.

---

## Errors

### 1. Invalid table index in `neon_shift_left()`
**File:** lib/net/net_crc_neon.c  
**Function:** `neon_shift_left()`

```c
uint8x16_t tbl = vld1q_u8(crc_neon_shift_tab + 16 - num);
```

**Problem:** When `num == 16`, this accesses `crc_neon_shift_tab[0]` which contains the pattern `0xff, 0xfe, ...`.
This is intended for shifting, but the function documentation states the parameter range is `0-16`.
When `num == 0`, it accesses `crc_neon_shift_tab[16]` which starts the right-shift pattern `0x00, 0x01, ...` -- this does NOT perform a zero-shift (identity operation), it performs a byte shuffle.

The table is 32 bytes, and `crc_neon_shift_tab + 16 - num` with `num` in `[0, 16]` accesses indices `[0, 16]`, which is valid in terms of bounds.
However, the semantics are wrong: `num == 0` should be a no-op (leave `reg` unchanged), but the table at offset 16 is the start of the *right-shift* pattern, not the identity pattern.

**Expected behavior:** `neon_shift_left(reg, 0)` should return `reg` unchanged.

**Suggested fix:**
- Document the actual valid range for `num` (appears to be `1-16` based on usage), OR
- Adjust the table layout or access pattern so that `num == 0` correctly returns the input unchanged

---

### 2. Incorrect constant ordering in `crcr32_folding_round()`
**File:** lib/net/net_crc_neon.c  
**Function:** `crcr32_folding_round()`

**Before patch:**
```c
vmull_p64(vgetq_lane_p64(...fold..., 1), vgetq_lane_p64(...precomp..., 0))
vmull_p64(vgetq_lane_p64(...fold..., 0), vgetq_lane_p64(...precomp..., 1))
```

**After patch:**
```c
vmull_p64(vgetq_lane_p64(...fold..., 0), vgetq_lane_p64(...precomp..., 0))
vmull_high_p64(...fold..., ...precomp...)
```

**Problem:** The patch changes which constant is multiplied with which fold lane, and simultaneously changes the constant order in `rte_net_crc_neon_init()` (swaps k1/k2 and k3/k4 elements).
The commit message states this is to "align constants to use PMULL and PMULL2 instructions instead of using cross lane operands" but does not provide correctness proof that the new pairing produces the same CRC result.

CRC folding algorithms depend on precise constant-to-operand pairing derived from the polynomial's mathematical properties.
Swapping which constant multiplies which operand changes the algorithm unless the constants were specifically reordered to compensate.

**Impact:** Without verification that the new constant order is mathematically equivalent for the CRC-32 and CRC-16-CCITT polynomials, this could silently produce incorrect CRC values.

**Suggested fix:**
- Add test results or reference to validation that the new constant pairing produces identical CRC output for known test vectors
- Document why the constant swap is algebraically correct, OR
- Provide before/after functional test output showing identical results

---

## Warnings

### 1. Missing bounds check on `num` parameter
**File:** lib/net/net_crc_neon.c  
**Function:** `neon_shift_left()`

```c
static inline uint64x2_t
neon_shift_left(uint64x2_t reg, const unsigned int num)
{
	uint8x16_t tbl = vld1q_u8(crc_neon_shift_tab + 16 - num);
	...
}
```

**Issue:** The function documentation states the valid range is `0-16`, but there is no bounds check.
If called with `num > 16`, the access `crc_neon_shift_tab + 16 - num` underflows and reads before the array start (undefined behavior).

**Suggested fix:**
```c
static inline uint64x2_t
neon_shift_left(uint64x2_t reg, const unsigned int num)
{
	RTE_ASSERT(num <= 16);
	uint8x16_t tbl = vld1q_u8(crc_neon_shift_tab + 16 - num);
	...
}
```
Or document that callers must ensure `num <= 16`.

---

### 2. Removal of intermediate mask in `crcr32_reduce_64_to_32()`
**File:** lib/net/net_crc_neon.c  
**Function:** `crcr32_reduce_64_to_32()`

**Before:**
```c
tmp1 = veorq_u64(tmp1, tmp0);
tmp1 = vandq_u64(tmp1, vld1q_u64((uint64_t *)mask1));  // masks upper 64 bits
tmp2 = vmull_p64(...tmp1..., ...);
tmp2 = veorq_u64(tmp2, tmp1);
tmp2 = veorq_u64(tmp2, tmp0);
```

**After:**
```c
tmp1 = veorq_u64(tmp1, tmp0);
tmp2 = vmull_p64(...tmp1..., ...);  // uses full 128-bit tmp1
tmp2 = veorq_u64(tmp2, tmp0);
```

**Issue:** The removed mask ensured that only the lower 64 bits of `tmp1` were used in the second multiplication.
The new code removes both the mask AND the `veorq_u64(tmp2, tmp1)` operation.
The x86 SSE reference likely uses an instruction that implicitly operates on 64 bits; verify that `vmull_p64(lane, lane)` on NEON only uses the specified lane and that removing the mask + XOR is mathematically correct.

**Suggested action:** Verify with test vectors that the output of `crcr32_reduce_64_to_32()` is unchanged, or document why the mask removal is safe.

---

### 3. Partial bytes logic complexity
**File:** lib/net/net_crc_neon.c  
**Function:** `crc32_eth_calc_pmull()` (partial bytes section)

**New code:**
```c
t1 = vld1q_u8(crc_neon_shift_tab + rem);
a  = vreinterpretq_u64_u8(vqtbl1q_u8(vreinterpretq_u8_u64(fold), t1));
t2 = vmvnq_u8(t1);
t2 = vqtbl1q_u8(vreinterpretq_u8_u64(fold), t2);
t1 = vcgezq_s8(vreinterpretq_s8_u8(t1));
b  = vreinterpretq_u64_u8(vbslq_u8(t1, last16, t2));
```

**Issue:** The logic uses `vcgezq_s8(t1)` to generate a mask.
The table `crc_neon_shift_tab + rem` contains byte indices; when interpreted as signed, indices `0x00-0x0f` are positive (mask bits = 0xff) and indices `0xf0-0xff` are negative (mask bits = 0x00).
This is clever, but the correctness depends on:
1. `rem` being in the correct range to index the transition point in the table
2. The blend selecting the right bytes from `last16` vs. `t2` (which is `vqtbl1q_u8(fold, ~t1)`)

The commit message states this "adapts x86 SSE logic" but does not provide a worked example or test output showing the partial-byte path produces correct results.

**Suggested action:** Add a comment explaining the mask generation logic, or provide test output showing correct CRC on odd-length inputs (e.g., 17, 33, 127 bytes).

---

### 4. Constant swap consistency
**File:** lib/net/net_crc_neon.c  
**Function:** `rte_net_crc_neon_init()`

**Changes:**
- `ccitt_k1_k2`: swapped from `{0x14ff2, 0x19a3c}` to `{0x19a3c, 0x14ff2}`
- `ccitt_k3_k4`: swapped from `{0x189ae, 0x8e10}` to `{0x8e10, 0x189ae}`
- `eth_k1_k2`: swapped
- `eth_k3_k4`: swapped

**Issue:** While the comment says this aligns with "PMULL and PMULL2 instructions," there is no explanation of *why* this ordering is correct.
CRC constants are polynomial-specific and order-sensitive.
The swap must be paired with the changed indexing in `crcr32_folding_round()` (now using lanes 0 and high instead of lanes 1 and 0).

**Suggested action:** Add a comment referencing the CRC polynomial or an external specification showing that the new (constant, operand) pairing is mathematically equivalent.

---

## Info

### 1. Removed `vshift_bytes_left()` function
The patch removes calls to `vshift_bytes_left()` (not shown in the diff, presumably in the original file) and replaces them with `neon_shift_left()`.
If `vshift_bytes_left()` is now unused, confirm it is removed from the file to avoid dead code.

---

## Positive Observations

1. **Cleaner table-based shift:** The `crc_neon_shift_tab` approach eliminates a 16-case switch statement, which is a good simplification.

2. **Alignment with x86 SSE:** Unifying the NEON and x86 code paths improves maintainability if the logic is verified correct.

3. **Removal of static arrays in reduction:** The `mask1`/`mask2` removal reduces data section size.

---

## Final Recommendation

**Do NOT merge** until:
1. The `neon_shift_left(num=0)` semantics are fixed or the valid range is clarified
2. Correctness of the constant swap is verified (test vectors, polynomial math, or reference)
3. The removed mask in `crcr32_reduce_64_to_32()` is proven safe

Consider adding:
- Unit tests for the partial-bytes path (odd-length inputs)
- Test vectors for CRC-32-Ethernet and CRC-16-CCITT covering 0-byte, 15-byte, 16-byte, 17-byte, 32-byte, and 33-byte inputs to validate the folding and reduction changes