On Fri, 30 Aug 2024 14:40:35 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding descriptive comments > > src/hotspot/share/opto/vectornode.cpp line 2159: > >> 2157: >> 2158: vmask_type = TypeVect::makemask(elem_bt, num_elem); >> 2159: mask = phase->transform(new VectorMaskCastNode(mask, vmask_type)); > > I would just have two variables, and not overwrite it: `integral_vmask_type` > and `vmask_type`. Maybe also `mask` could be split into two variables? I think the variable names are appropriate and in accordance with convention. > src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java line > 2770: > >> 2768: >> 2769: /** >> 2770: * Rearranges the lane elements of two vectors, selecting lanes > > I have a bit of a name concern here. Why are we calling it "select" and not > "rearrange"? Because for a single "from" vector we also call it "rearrange", > right? Is "select" not often synonymous to "blend", which works also with two > "from" vectors, but with a mask and not indexing for "selection/rearranging"? We already have another flavor of [selectFrom](https://docs.oracle.com/en/java/javase/22/docs/api/jdk.incubator.vector/jdk/incubator/vector/Vector.html#selectFrom(jdk.incubator.vector.Vector)) which permutes single vector, new API extents its semantics to two vector selection, so we kept the nomenclature consistent. > test/jdk/jdk/incubator/vector/Byte128VectorTests.java line 324: > >> 322: boolean is_exceptional_idx = (int)order[idx] >= >> vector_len; >> 323: int oidx = is_exceptional_idx ? ((int)order[idx] - >> vector_len) : (int)order[idx]; >> 324: Assert.assertEquals(r[idx], (is_exceptional_idx ? >> b[i + oidx] : a[i + oidx])); > > I thought general Java style is camelCase? Is that not followed in the > VectorAPI code? I agree, but somehow we are using non camelCase conventions in this file, look for uses of 'vector_len'. just preserving file level convention. > test/jdk/jdk/incubator/vector/ShortMaxVectorTests.java line 1048: > >> 1046: return >> SHORT_GENERATOR_SELECT_FROM_TRIPLES.stream().map(List::toArray). >> 1047: toArray(Object[][]::new); >> 1048: } > > Just a control question: does this also occasionally generate examples with > out-of-bounds indices? Negative out of bounds and positive out of bounds? Original API did throw IndexOutOfBoundsException, but later on we have moved away from exception throwing semantics to wrapping semantics. Please find details at following comment https://github.com/openjdk/jdk/pull/20508#issuecomment-2306344606 > test/jdk/jdk/incubator/vector/ShortMaxVectorTests.java line 5812: > >> 5810: ShortVector bv = ShortVector.fromArray(SPECIES, b, i); >> 5811: ShortVector idxv = ShortVector.fromArray(SPECIES, idx, >> i); >> 5812: idxv.selectFrom(av, bv).intoArray(r, i); > > Would this test catch a bug where the backend would generate vectors that are > too long or too short? Existing vectorAPI inline expansion entry points explicitly pass lane type and count as intrinsic arguments, this is used to create concrete ideal vector types. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1747532692 PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1747532456 PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1747532419 PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1747532340 PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1747532307
