Integrate SimdUnicode UTF-8 Validation for AdvSimd - #122090
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
contv and n4v are Vector128<sbyte> so the largest positive value is 127.
|
pinging @tannergooding FYI for libraries change |
|
@tannergooding This PR needs your review this coming week please. |
| else | ||
| { | ||
| numContinuationBytes += Vector128.CountWhereAllBitsSet(byte2High); | ||
| numFourByteSequences += Vector128.CountWhereAllBitsSet(Vector128.SubtractSaturate(currentBlock, fourthByte)); | ||
| } |
There was a problem hiding this comment.
This looks like currently dead code? Any reason it can't just be the code used everywhere?
There was a problem hiding this comment.
This should be the default algorithm for other architectures, but it is particularly slow on AdvSimd (due to the lack of ExtractMostSignificantBits) as it is much faster to use AddAcross. Since only AdvSimd is enabled currently, this will be dead code. Should I comment them out so that they can be added back when other architectures are eventually enabled?
There was a problem hiding this comment.
I think we want to just avoid dead code in general and we want to otherwise prioritize the JIT (or libraries implementation) optimizing CountWhereAllBitsSet for Arm64
There was a problem hiding this comment.
A viable way to handle this, for example, would be to just use CountWhereAllBitsSet and then update the managed implementation to use AddAcross on Arm64
There was a problem hiding this comment.
I'm not sure if it fits in this case, AdvSimd requires a vector accumulator outside the loop that only updates when it overflows, otherwise doing AddAcross on every iteration would be very slow. This seems to be a very different approach than using CountWhereAllBitsSet, which is what the AdvSimd path is trying to bypass.
There was a problem hiding this comment.
CountWhereAllBitsSet was since explicitly optimized for Arm64: #126678
Could you try the perf again and share the numbers on the difference between just using the xplat logic (CountWhereAllBitsSet) vs the custom Arm64 handling here?
You may be able to explicitly utilize the internal CountMatches as well, if you are able to avoid the Equals(vector, AllBitsSet) because the input is known to be 0 or allbitsset on a per-element basis.
There was a problem hiding this comment.
Here are the results where the "Vector128" results are using CountMatches:
| Method | Input | Version | Mean | Error | Ratio |
|---|---|---|---|---|---|
| GetCharCount | EnglishAllAscii | Original | 2.199 us | 0.0188 us | 1.000 |
| GetCharCount | EnglishAllAscii | Custom AdvSimd | 2.198 us | 0.0046 us | 1.000 |
| GetCharCount | EnglishAllAscii | Vector128 | 2.197 us | 0.0066 us | 0.999 |
| GetCharCount | EnglishMostlyAscii | Original | 31.391 us | 0.1939 us | 1.000 |
| GetCharCount | EnglishMostlyAscii | Custom AdvSimd | 12.095 us | 0.0933 us | 0.385 |
| GetCharCount | EnglishMostlyAscii | Vector128 | 15.012 us | 0.0220 us | 0.478 |
| GetCharCount | Chinese | Original | 35.132 us | 0.2354 us | 1.000 |
| GetCharCount | Chinese | Custom AdvSimd | 22.310 us | 0.1520 us | 0.635 |
| GetCharCount | Chinese | Vector128 | 35.947 us | 0.2294 us | 1.023 |
| GetCharCount | Cyrillic | Original | 25.100 us | 0.5019 us | 1.000 |
| GetCharCount | Cyrillic | Custom AdvSimd | 11.687 us | 0.0539 us | 0.466 |
| GetCharCount | Cyrillic | Vector128 | 18.630 us | 0.0942 us | 0.742 |
| GetCharCount | Greek | Original | 62.923 us | 0.7590 us | 1.000 |
| GetCharCount | Greek | Custom AdvSimd | 15.637 us | 0.1152 us | 0.249 |
| GetCharCount | Greek | Vector128 | 24.982 us | 0.2431 us | 0.397 |
There was a problem hiding this comment.
Pull request overview
This PR integrates a SimdUnicode-inspired UTF-8 validation fast path for Arm64 AdvSimd into Utf8Utility.GetPointerToFirstInvalidByte, aiming to improve throughput on mixed / non-ASCII inputs while reusing existing ASCII-vector helpers and expanding unit test coverage.
Changes:
- Add an Arm64
Vector128-based UTF-8 validation path (SimdUnicode “lookup” algorithm) toUtf8Utility.GetPointerToFirstInvalidByte. - Expose
Ascii.VectorContainsNonAsciiChar(Vector128<byte>)for reuse by the new validator. - Expand UTF-8 validation tests to exercise more insertion positions and add additional out-of-range 4-byte start byte coverage; add SimdUnicode to third-party notices.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/Unicode/Utf8UtilityTests.ValidateBytes.cs | Broaden invalid-sequence test coverage by inserting invalid sequences at more positions; adds F5..FF coverage. |
| src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf8Utility.Validation.cs | Adds the Arm64 Vector128 SimdUnicode-style validator and routes to it for sufficiently large inputs. |
| src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs | Makes VectorContainsNonAsciiChar(Vector128<byte>) internal so it can be reused by UTF-8 validation. |
| THIRD-PARTY-NOTICES.TXT | Adds SimdUnicode MIT license notice. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "04be6a73f3670d2731f31d83a4e14b66beaf3342",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "c89371d5294a9be40b263b275c58cf0b2a73ea95",
"last_reviewed_commit": "04be6a73f3670d2731f31d83a4e14b66beaf3342",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "c89371d5294a9be40b263b275c58cf0b2a73ea95",
"last_recorded_worker_run_id": "29684904166",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "04be6a73f3670d2731f31d83a4e14b66beaf3342",
"review_id": 4730680252
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. UTF-8 validation (Utf8Utility.GetPointerToFirstInvalidByte, the core of Encoding.UTF8 decoding, Utf8.IsValid, etc.) currently runs a scalar DWORD-at-a-time loop on Arm64. The PR ports the well-known SimdUnicode/lookup4 algorithm (Keiser & Lemire) to AdvSimd.Arm64, and the benchmark table shows large wins on non-ASCII inputs (e.g. Greek GetCharCount ~4x faster on Neoverse-V2/N2) with no ASCII regression. Contributes to #103781.
Approach: Reasonable and consistent with how this file already handles Arm64 (GetNonAsciiBytes, AddPairwise). The new GetPointerToFirstInvalidByte_Vector128 is dispatched only when AdvSimd.Arm64.IsSupported && BitConverter.IsLittleEndian && inputLength >= Vector128<byte>.Count, so all other platforms/endianness keep the existing scalar path. Reuse of Ascii.VectorContainsNonAsciiChar (newly promoted to internal) and encapsulation of the intrinsic-specific pieces behind if (AdvSimd.Arm64.IsSupported) to ease future Vector128.Shuffle-based generalization are both sensible. Correctness of the returned pointer and the utf16CodeUnitCountAdjustment/scalarCountAdjustment outputs depends heavily on subtle counting logic (vecContinuationBytes/vecFourByteSequences accumulation, the 0x7f overflow drain, and the AdjustCounters/SimpleRewindAndValidateWithErrors rewind), which is hard to verify by reading alone.
Summary: else { throw PlatformNotSupportedException(); } branches / CountWhereAllBitsSet question near line 996, run-on formatting, += -x readability, variable naming, and the ASCII fast-path prevInputBlock question) have already been raised and discussed by @tannergooding, @lemire, and Copilot; I am deliberately not duplicating those threads. Items below are for reviewer attention, not new blocking findings.
Detailed Findings
✅ Dispatch guard and platform isolation — correct
The new fast path is entered only for little-endian Arm64 with inputLength >= Vector128<byte>.Count, after ASCII draining. The scalar fallback remains intact for every other case, so this is purely additive for non-Arm64 targets. Endianness gating is appropriate given the byte-nibble table lookups.
⚠️ Test coverage exercises the new path only on Arm64 hardware
The reworked AssertIsInvalid{Two,Three,Four}ByteSequence helpers now insert the invalid sequence at positions 0..16 to straddle 16-byte vector boundaries, which is a good improvement over the old fixed-offset cases. However, GetPointerToFirstInvalidByte_Vector128 is only reached on little-endian Arm64. On x64/x86 CI these tests still validate correct results, but they do not execute the new SIMD code — so the algorithm (adjustment counters, rewind, 0x7f overflow drain, the 4-block ASCII unroll) is only covered by CI legs that actually run on Arm64. Worth confirming the Arm64 test legs are green and that inputs long enough to trigger the overflow-drain and unroll branches are represented. Observation for reviewer attention, not a request to change tests.
✅ THIRD-PARTY-NOTICES and attribution — present and appropriate
The SimdUnicode MIT license text and the arXiv paper attribution are added to THIRD-PARTY-NOTICES.TXT, and the source carries an attribution comment. This is the right handling for a derived algorithm.
💡 Counting/rewind logic is the highest-risk area to validate
CalculateN2N3FinalSIMDAdjustments, AdjustCounters (over/under-count correction across processedIndex/invalidIndex), and SimpleRewindAndValidateWithErrors (the howFarBack/numIncomplete back-scan) together determine the returned pointer and both out adjustments on the error and trailing-incomplete paths. The back-scan indexes buffer[startIndex - i]; it appears safe because numIncomplete == 3 only after a prior non-ASCII block (so processedLength >= 16), but this invariant is implicit and easy to break in future edits — a Debug.Assert on startIndex - howFarBack >= 0 would make it robust. Recommend a maintainer trace these paths against the paper's reference semantics.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 222.7 AIC · ⌖ 11 AIC · ⊞ 10K
Contributes to #103781, based on #104199.
Includes some changes to reduce code duplication and reuse existing code such as
VectorContainsNonAsciiCharto fit in with the library. Most intrinsics are replaced with Vector128 APIs. Otherwise, I tried to encapsulate where platform-specific intrinsics are used so that it will be easier to extend to other Vector128 platforms. That would just require replacingAdvSimd.Arm64.VectorTableLookupwithVector128.Shuffleand some Vector128 replacement forAdvSimd.ExtractVector128, which can be done in a later PR.The unit tests are also modified to improve the coverage and test cases.
Benchmark results
Neoverse-V2:
Neoverse-N2:
cc @dotnet/arm64-contrib @lemire @EgorBo @a74nh @SwapnilGaikwad