On Thu, 26 Mar 2026 13:38:28 GMT, Emanuel Peter <[email protected]> wrote:
>> Found while working on JDK-8369699. >> >> In general, the Vector API documentation is a bit vague around part numbers. >> There was considerable confusion around expansion/contraction: are we >> talking about logical, physical or output expansion/contraction? >> Confusingly, in some places we called expansions things that go from more to >> fewer bits, and we called contractions that went from fewer to more bits. >> >> And exception messages are not very helpful, for example they don't provide >> the legal range. >> >> @rose00 took a first stab at improving things >> (https://github.com/openjdk/jdk/pull/29306), and I eventually took over the >> project. >> >> -------------------------------------- >> >> Principles: >> - Expansion means fewer->more bits. >> - Contraction means more->fewer bits. >> - Be clear about input, logical result and output. >> - We primarily use: >> - conversion lanewise expansion (logical) >> - conversion lanewise contraction (logical) >> - conversion lanewise in-place (logical) >> - reinterpret (logical) >> - select, for truncation (output) >> - insert, for padding (output) >> - in-place, logical fits output (output) >> >> Please review this PR in this order: >> - Changes in the "Expansions, contractions, and partial results" section of >> `Vector.java`. We must first agree on the definitions here, before we go and >> disagree elsewhere ;) >> - Changes in affected methods `convertShape`, `convert`, and >> `reinterpretShape`. >> - Internal changes in `AbstractVector.java`: adjust nomenclature and >> exception message. >> - New test. I think it is necessary, I caught some mistakes I made. And when >> I wanted to add tests for `unslice` I realized that it does not throw for >> out of bounds `part`. So I think it is justified. >> >> In general, I'm a bit worried that the documentation is a bit too long, and >> feels a bit heavy/overwhelming. >> To a large degree, this is due to the complexity of part numbers. >> We could drop some paragraphs and some repetition. Let me know what you >> think is too much. >> More explanations may help make things clearer, but also risk being too much >> and overwhelming. >> I'm open to cut things down more, and any other constructive suggestions ;) >> >> ----------------------- >> >> While reading the documentation and testing for `unslice`, I found out that >> https://github.com/openjdk/jdk/pull/3804 accidentally removed the bounds >> checks for `part`. But we did not notice, because there were no tests for >> it. I'm adding the bounds check back in and adding tests ... > > Emanuel Peter has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 33 additional > commits since the last revision: > > - Merge branch 'master' into > JDK-8375631-part-number-exception-and-documentation > - more test > - wip part number for unslice > - fix up documentation a little more > - more small updates > - fix small issues > - small fix in test and assert code > - Merge branch 'master' into > JDK-8375631-part-number-exception-and-documentation > - more testing > - cleanup and testReinterpret > - ... and 23 more: https://git.openjdk.org/jdk/compare/a7f2078b...ee08c9ef src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractVector.java line 505: > 503: int resSizeLog2 = domSizeLog2 + laneChangeLog2; > 504: // resSizeLog2 = 0 => 1-lane vector shrinking to 1-byte lane-size > 505: // resSizeLog2 < 0 => small vector shrinking by more than a > lane-size Do you mean `resSizeLog2 > 0 ...`? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractVector.java line 573: > 571: return 0 <= part && part < limit; > 572: } else if (limit < 0) { // insertion (output is logical result > with padding) > 573: return limit < part && part <= 0; `return limit <= part ...` ? since you flipped the operands. test/jdk/jdk/incubator/vector/PartNumberTest.java line 45: > 43: } > 44: > 45: interface TestMethod { Annotate with `@FunctionalInterface`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3003008445 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3003018471 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3003090067
