Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li wrote: > Hi, > Can you help to review this simple patch? > Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not > necessary, could be removed. > Thanks The intrinsic implementation will not perform bounds checks. I think what you may be observing is the result of bounds checks when Java code is interpreted (or perhaps from compiled C1 code). You need to first ensure the code is compiled to C2 before executing with out of bounds values. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2125465612
Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li wrote: > Hi, > Can you help to review this simple patch? > Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not > necessary, could be removed. > Thanks In one side, yes, there is a gap in the tests. In another side, I wonder if the check here is necessary (i.e. `VectorIntrinsics.checkIndex(vix, a.length)`). * For default java implementation, it's not necessary, as java code will check it anyway; * for intrinsic implementation, I saw relative information (array, offset, index map, offset in the map) are wrapped in LoadVectorGatherNode, I wonder it will also check IOOBE? As I totally removed `VectorIntrinsics.checkIndex(vix, a.length);`, and in either conditions it still throws IOOBE when it should throw IOOBE. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2125320559
Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Mon, 20 May 2024 19:21:44 GMT, Paul Sandoz wrote: >> Hi, >> Can you help to review this simple patch? >> Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not >> necessary, could be removed. >> Thanks > > That does not look correct and will only check a prefix indexes. A > `ByteVector` with a shape of 256 bits has 32 lanes, whereas an `IntVector` of > the same shape has 8 lanes. The `mapOffset` array will hold 32 indexes that > need checking, so we need to loop through `mapOffset` array four times. > Thanks @PaulSandoz for comment. I just re-ran the vector api tests with this > patch on x64 and riscv64, but seems no failures triggered. Let me check > further, either I missed something or maybe there is some gap in test to be > filled. More likely a gap in the tests, not sufficiently checking for out of bounds access across the range in the mapOffset array. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2122917109
Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Mon, 20 May 2024 19:21:44 GMT, Paul Sandoz wrote: >> Hi, >> Can you help to review this simple patch? >> Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not >> necessary, could be removed. >> Thanks > > That does not look correct and will only check a prefix indexes. A > `ByteVector` with a shape of 256 bits has 32 lanes, whereas an `IntVector` of > the same shape has 8 lanes. The `mapOffset` array will hold 32 indexes that > need checking, so we need to loop through `mapOffset` array four times. Thanks @PaulSandoz for comment. I just re-ran the vector api tests with this patch on x64 and riscv64, but seems no failures triggered. Let me check further, either I missed something or maybe there is some gap in test to be filled. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2122240453
Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li wrote: > Hi, > Can you help to review this simple patch? > Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not > necessary, could be removed. > Thanks That does not look correct and will only check a prefix indexes. A `ByteVector` with a shape of 256 bits has 32 lanes, whereas an `IntVector` of the same shape has 8 lanes. The `mapOffset` array will hold 32 indexes that need checking, so we need to loop through `mapOffset` array four times. - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2121057883
Re: RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
On Fri, 26 Apr 2024 14:06:02 GMT, Hamlin Li wrote: > Hi, > Can you help to review this simple patch? > Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not > necessary, could be removed. > Thanks @PaulSandoz who would be a good person to give a review on this one? It's not architecture specific. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18977#issuecomment-2119954616
RFR: 8331196: vector api: Remove unnecessary index check in Byte/ShortVector.fromArray/fromArray0Template
Hi, Can you help to review this simple patch? Some index check in Byte/ShortVector.fromArray/fromArray0Template seems not necessary, could be removed. Thanks - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/18977/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18977=00 Issue: https://bugs.openjdk.org/browse/JDK-8331196 Stats: 36 lines in 3 files changed: 0 ins; 12 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/18977.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18977/head:pull/18977 PR: https://git.openjdk.org/jdk/pull/18977
