On Tue, 31 Mar 2026 10:13:14 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 incrementally with one additional > commit since the last revision: > > fix small typo src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 825: > 823: * produces a <em>logical result</em> that is too large for the > 824: * required physical output {@code VSHAPE}, so a part of the logical > 825: * output must be <em>selected</em> to deliver into the physical output. Suggestion: * required physical output {@code VSHAPE}, so a part of the logical * result must be <em>selected</em> to deliver into the physical output. ? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 850: > 848: * <p> In a nutshell, we would prefer to keep both {@code VSHAPE} and > 849: * {@code VLENGTH} constant in a block of vector code, but size > 850: * changes sometimes require messy compromises. Here are some Suggestion: * changes sometimes require compromises. Here are some src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 854: > 852: * > 853: * <ul><li>The <em>input shape</em> of a vector operation is the > 854: * shape of the vector object {@code X} which receives the method call. Suggestion: * shape of the input vector {@code X} which receives the method call. src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 858: > 856: * > 857: * </li><li>The <em>physical output shape</em> of a vector operation > 858: * is the shape of the output container, the vector {@code Y} produced Suggestion: * is the shape of the output, the vector {@code Y} produced src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 882: > 880: * vector operation is the bit-size ratio {@code |f(X)|/|X|} of the > 881: * logical result to the input shape. It may be an integer or the > 882: * reciprocal of an integer, depending on whether the logical Suggestion: * reciprocal of an integer, respectively depending on whether the logical src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 885: > 883: * operation is expanding or contracting. > 884: * > 885: * </li><li>Given an output container {@code Y} of a specific shape, Suggestion: * </li><li>Given an output vector {@code Y}, src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 910: > 908: * > 909: * </li><li><em>Selection</em> ({@code MS>1}, {@code MO<1}) occurs > 910: * when the output container {@code Y} is smaller than the logical Suggestion: * when the physical output shape, the shape of vector {@code Y}, is smaller than the logical src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 923: > 921: * > 922: * </li><li><em>Insertion</em> ({@code MO>1}, {@code MS<1}) occurs > 923: * when the output container {@code Y} is larger than the logical Suggestion: * when the physical output shape, the shape of vector {@code Y}, is larger than the logical src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 931: > 929: * {@code L} is the {@code VLENGTH} of {@code f(X)} and > 930: * {@code R=|part|*L}. > 931: * If the user wants a fully populated output shape, the operation Suggestion: * If the user wants a fully populated output vector, the operation src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 940: > 938: * <ul> > 939: * <li> Conversion {@code int[4]:128 -> float[4]:128}: > 940: * invariant lane size (lanewise in-place, {@code ML=1}), Suggestion: * length-invariant (lanewise in-place, {@code ML=1}), ? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line 1039: > 1037: * <li> Bitwise Reinterpretation: > 1038: * {@code f(X)} is the bitwise image of {@code X} of the same vector > 1039: * bit size, only the lane boundaries are redrawn from {@code ETYPE} Suggestion: * {@code f(X)} is the bitwise image of {@code X} of the same shape, * only the lane boundaries are redrawn from {@code ETYPE} ? In that respect I think we may have some incorrect entries for rows of `reinterpretShape` and the logical result, which should be of the same shape. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3017946451 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3017955340 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3017993579 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3017995717 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3017978109 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018014153 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018050997 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018053840 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018073982 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018112015 PR Review Comment: https://git.openjdk.org/jdk/pull/30113#discussion_r3018235747
