Repository navigation
Repros 755 - #757
Closed
jshook wants to merge 4 commits into
Closed
Repros 755#757jshook wants to merge 4 commits into
jshook wants to merge 4 commits into
Conversation
jshook
requested review from
MarkWolters,
ashkrisk and
tlwillke
as code owners
October 14, 2026 01:33
Contributor
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
Collaborator
|
Thanks for the review and the PR. I will close since the test issues were addressed and the other items were also either already identified or were minor misunderstandings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review findings: repros and fixes
These were reviewed at
d903d206. The repro tests and proposed fixes are onrepros-755, one commit each. Both blockers are the PR's own new tests failing, and together they account for the red CI. The kernel rewrite itself checks out as equivalent: same lane grouping, accumulation and reduction order, tails and offsets, and noClassCastExceptionunder the native provider.TestNVQEncodingAllocationfails intermittently: bitwise reduction checks depend on vector width and JIT timing5fd4e4620ea4ef0aTestDotProduct256.exactWidth…fails on every host with 128-bit vectors (ARM NEON)e25e8ba29dc03cd91. The test compares the reduction results of
nvqLoss,nvqUniformLoss,nvqSquareL2Distance8bitandnvqCosine8bitbit for bit against a reference subclass. Both sides end inreduceLanes(ADD), and the Vector API leaves the order of those additions unspecified. So the interpreter and C1 fallback can round differently from the C2 intrinsic, and which side runs at which tier varies from run to run. At 256-bit or 128-bit width this test fails by 1 ULP in 60–80% of fresh JVMs, and it fails occasionally at 512-bit too. The fix keeps the encoded bytes exact and compares the reductions within 1e-5 relative.2. The expected value is a single 8-lane
SPECIES_256reduction. With 4 lanes, length 8 goes throughdotProductPreferred, which adds in a different order, as main does too. The fix skips the check when the preferred species has fewer than 8 lanes.With both fixes, the jvector-twenty tests pass at 512-bit, 256-bit and 128-bit width; at 128-bit, the exact-width check is skipped as intended. The narrower widths can be forced with
JAVA_TOOL_OPTIONS=-XX:MaxVectorSize=32or=16.Not fixed, worth addressing:
TestNVQEncodingAllocationis a bitwise comparison with no allocation measurement, so nothing would catch the temporary allocations returning.201dd576isn't main, and the measured commitcb0f5fb5doesn't exist in the repo. Each cell is a single run.