8375631: VectorAPI part number: reformulate documentation and improve exception message#30113
8375631: VectorAPI part number: reformulate documentation and improve exception message#30113eme64 wants to merge 30 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/contributor add @rose00 |
|
@eme64 |
|
This is looking good so far. Obviously I agree with the definitions. I like this useful parallelism:
Good improvement to Very good test code. Unslice should throw, clearly. Doing the same for all non-zero part numbers was an oversight. I agree that it is very hard to balance the goals of completeness and "lightness" of presentation.
Until we have more reliable vector size changes (depending on synthetics) we are stuck with part numbers. We might as well explain them fully in the javadoc, which is the official API spec. We could add an essay or white paper off to the side somewhere which explains the part numbers at length. That could relocate some of the "heaviness" elsewhere. In a possible future version of the API, we can delete the part numbers, and not the "why don’t you just" move of defaulting them to zero, which papers over the size-change problem without addressing it at the root. The future API version can add part-free API points which simply DTRT and produce a vector, possibly synthetic, of the right size. That will require JIT work to avoid having this simpler user model break performance expectations completely. More recent vector ISAs seem to be moving in this direction, of allowing flexible physical size changes in close harmony with logical size changes. In the possible future, maybe that "part number essay" could carry more of the weight, since part numbers would not be in every user’s face the way they must be today. |
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 (#29306), and I eventually took over the project.
Principles:
Please review this PR in this order:
Vector.java. We must first agree on the definitions here, before we go and disagree elsewhere ;)convertShape,convert, andreinterpretShape.AbstractVector.java: adjust nomenclature and exception message.unsliceI realized that it does not throw for out of boundspart. 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 #3804 accidentally removed the bounds checks forpart. But we did not notice, because there were no tests for it. I'm adding the bounds check back in and adding tests for it as well.Progress
Issue
Contributors
<jrose@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30113/head:pull/30113$ git checkout pull/30113Update a local copy of the PR:
$ git checkout pull/30113$ git pull https://git.openjdk.org/jdk.git pull/30113/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30113View PR using the GUI difftool:
$ git pr show -t 30113Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30113.diff
Using Webrev
Link to Webrev Comment