Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v4]
> This PR proposes removing the restriction that only heap `MemorySegment` > wrapping a `byte` array can be accessed by Vectors. Now any array type can be > used provided the element alignment constraints are respected. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Remove IntrinsicHeapTest - Changes: - all: https://git.openjdk.org/jdk/pull/16360/files - new: https://git.openjdk.org/jdk/pull/16360/files/c104051c..7f4a1871 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16360=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16360=02-03 Stats: 160 lines in 1 file changed: 0 ins; 160 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16360.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16360/head:pull/16360 PR: https://git.openjdk.org/jdk/pull/16360
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v3]
On Mon, 30 Oct 2023 13:43:13 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Add benchmark and intrinsification spy Looks good. Please remove `IntrinsicHeapTest` and let's follow up in another issue/PR to fix the mismatched heap access not being intrinsic. - Marked as reviewed by psandoz (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16360#pullrequestreview-1704600236
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v3]
On Mon, 30 Oct 2023 13:43:13 GMT, Per Minborg wrote:
>> This PR proposes removing the restriction that only heap `MemorySegment`
>> wrapping a `byte` array can be accessed by Vectors. Now any array type can
>> be used provided the element alignment constraints are respected.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add benchmark and intrinsification spy
test/jdk/jdk/incubator/vector/IntrinsicHeapTest.java line 48:
> 46: import static org.testng.Assert.*;
> 47:
> 48: public class IntrinsicHeapTest {
I don't think we need this test as part of this PR. It's useful ascertain what
is intrinsic or not. We can rethink it as part of the following fix if needed
to verify the intrinsics work across all the cross product of array types and
vector types.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1376494655
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v3]
> This PR proposes removing the restriction that only heap `MemorySegment` > wrapping a `byte` array can be accessed by Vectors. Now any array type can be > used provided the element alignment constraints are respected. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add benchmark and intrinsification spy - Changes: - all: https://git.openjdk.org/jdk/pull/16360/files - new: https://git.openjdk.org/jdk/pull/16360/files/4828a98b..c104051c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16360=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16360=01-02 Stats: 420 lines in 3 files changed: 419 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16360.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16360/head:pull/16360 PR: https://git.openjdk.org/jdk/pull/16360
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Thu, 26 Oct 2023 09:17:25 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Allow unaligned array access Here is a test showing there is a need to improve performance for certain combinations of load operations. It is likely, the same is true for store operations. Benchmark (size) Mode Cnt Score Error Units TestLoadSegmentVarious.byteVectorFromByteBackedSegment1024 avgt 10 280.008 ? 7.251 ns/op TestLoadSegmentVarious.byteVectorFromDoubleBackedSegment 1024 avgt 10 1304.008 ? 98.901 ns/op TestLoadSegmentVarious.byteVectorFromIntBackedSegment 1024 avgt 10 1279.621 ? 100.008 ns/op TestLoadSegmentVarious.doubleVectorFromByteBackedSegment 1024 avgt 10 37.281 ? 1.360 ns/op TestLoadSegmentVarious.doubleVectorFromDoubleBackedSegment1024 avgt 10 36.847 ? 0.130 ns/op TestLoadSegmentVarious.doubleVectorFromIntBackedSegment 1024 avgt 10 194.195 ? 31.096 ns/op TestLoadSegmentVarious.intVectorFromByteBackedSegment 1024 avgt 10 72.602 ? 1.768 ns/op TestLoadSegmentVarious.intVectorFromDoubleBackedSegment 1024 avgt 10 166.851 ? 9.528 ns/op TestLoadSegmentVarious.intVectorFromIntBackedSegment 1024 avgt 10 71.283 ? 0.507 ns/op TestLoadSegmentVarious.scalarByteVectorFromByteSegment1024 avgt 10 4790.084 ? 45.882 ns/op TestLoadSegmentVarious.scalarByteVectorFromDoubleSegment 1024 avgt 10 4841.273 ? 291.962 ns/op TestLoadSegmentVarious.scalarByteVectorFromIntSegment 1024 avgt 10 4794.028 ? 101.282 ns/op TestLoadSegmentVarious.scalarDoubleVectorFromByteSegment 1024 avgt 10 1241.117 ? 11.603 ns/op TestLoadSegmentVarious.scalarDoubleVectorFromDoubleSegment1024 avgt 10 1245.752 ? 15.516 ns/op TestLoadSegmentVarious.scalarDoubleVectorFromIntSegment 1024 avgt 10 1232.216 ? 8.365 ns/op TestLoadSegmentVarious.scalarIntVectorFromByteSegment 1024 avgt 10 1239.146 ? 14.582 ns/op TestLoadSegmentVarious.scalarIntVectorFromDoubleSegment 1024 avgt 10 1236.712 ? 8.063 ns/op TestLoadSegmentVarious.scalarIntVectorFromIntSegment 1024 avgt 10 1228.656 ? 3.329 ns/op As can be seen, vector performance for `IntVector` and `DoubleVector` is good for operations from a byte array or where the array types match. This work can be made under a separate issue. - PR Comment: https://git.openjdk.org/jdk/pull/16360#issuecomment-1785186113
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Thu, 26 Oct 2023 09:17:25 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Allow unaligned array access We also need to review the intrinsic for load/store found here to ensure, when the hardware supports it, that mixed access is optimized: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/vectorIntrinsics.cpp#L967 I recommend writing a simple benchmark with various forms of mixed access and verify the intrinsics kick in (longer term IR tests would be preferable). - PR Comment: https://git.openjdk.org/jdk/pull/16360#issuecomment-1781482409
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Thu, 26 Oct 2023 09:17:25 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Allow unaligned array access Marked as reviewed by mcimadamore (Reviewer). On a second look, I agree with the comments. Logically, the vector API is accessing elements using `JAVA_XYZ_UNALIGNED` layouts, which should _always_ succeed. E.g. no alignment exception should ever be possible when doing unaligned access, regardless of whether the segment is on/off heap. - PR Review: https://git.openjdk.org/jdk/pull/16360#pullrequestreview-1700040222 PR Comment: https://git.openjdk.org/jdk/pull/16360#issuecomment-1781433386
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Thu, 26 Oct 2023 09:17:25 GMT, Per Minborg wrote: >> This PR proposes removing the restriction that only heap `MemorySegment` >> wrapping a `byte` array can be accessed by Vectors. Now any array type can >> be used provided the element alignment constraints are respected. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Allow unaligned array access LGTM - Marked as reviewed by chegar (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16360#pullrequestreview-1699260449
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
On Wed, 25 Oct 2023 16:35:30 GMT, Chris Hegarty wrote:
>> test/jdk/jdk/incubator/vector/AbstractVectorLoadStoreTest.java line 118:
>>
>>> 116: private static boolean canBeConverted(IntFunction
>>> function, ValueLayout elementLayout) {
>>> 117: // Create a sample to analyze
>>> 118: MemorySegment s = function.apply(Long.BYTES);
>>
>> I believe that a good way to test this is the following:
>> * each vector type operates as having a given element layout - for instance,
>> you can imagine the layout for IntVector to be `JAVA_INT` and so forth
>> * asking whether you load a segment into a vector is the same as asking
>> whether you can access the segment, at offset 0L, with the layout associated
>> with the vector (see above) - that is if MS::get throws, then vector load
>> should also throw (and viceversa)
>
> Vectors use and have in their docs, a layout with a byte alignment of 1. e.g.
> from `IntVector`
>
>
> ValueLayout.OfInt ELEMENT_LAYOUT = ValueLayout.JAVA_INT.withByteAlignment(1)
>
>
> Can the `fromMemorySegment` not just behave similarly? I get that alignment
> is preferable, but does it need to be enforced? If so, then maybe the
> `ELEMENT_LAYOUT` and example in the javadoc needs to be updated.
I've removed the alignment checks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1372844356
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[] [v2]
> This PR proposes removing the restriction that only heap `MemorySegment` > wrapping a `byte` array can be accessed by Vectors. Now any array type can be > used provided the element alignment constraints are respected. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Allow unaligned array access - Changes: - all: https://git.openjdk.org/jdk/pull/16360/files - new: https://git.openjdk.org/jdk/pull/16360/files/75bd88ca..4828a98b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16360=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16360=00-01 Stats: 326 lines in 41 files changed: 137 ins; 99 del; 90 mod Patch: https://git.openjdk.org/jdk/pull/16360.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16360/head:pull/16360 PR: https://git.openjdk.org/jdk/pull/16360
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 13:08:06 GMT, Per Minborg wrote: > This PR proposes removing the restriction that only heap `MemorySegment` > wrapping a `byte` array can be accessed by Vectors. Now any array type can be > used provided the element alignment constraints are respected. Vector loads and stores are always unaligned, so I don't see the value in forcing the alignment of accesses. - PR Comment: https://git.openjdk.org/jdk/pull/16360#issuecomment-1779674689
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 14:01:09 GMT, Maurizio Cimadamore
wrote:
>> This PR proposes removing the restriction that only heap `MemorySegment`
>> wrapping a `byte` array can be accessed by Vectors. Now any array type can
>> be used provided the element alignment constraints are respected.
>
> test/jdk/jdk/incubator/vector/AbstractVectorLoadStoreTest.java line 118:
>
>> 116: private static boolean canBeConverted(IntFunction
>> function, ValueLayout elementLayout) {
>> 117: // Create a sample to analyze
>> 118: MemorySegment s = function.apply(Long.BYTES);
>
> I believe that a good way to test this is the following:
> * each vector type operates as having a given element layout - for instance,
> you can imagine the layout for IntVector to be `JAVA_INT` and so forth
> * asking whether you load a segment into a vector is the same as asking
> whether you can access the segment, at offset 0L, with the layout associated
> with the vector (see above) - that is if MS::get throws, then vector load
> should also throw (and viceversa)
Vectors use and have in their docs, a layout with a byte alignment of 1. e.g.
from `IntVector`
ValueLayout.OfInt ELEMENT_LAYOUT = ValueLayout.JAVA_INT.withByteAlignment(1)
Can the `fromMemorySegment` not just behave similarly? I get that alignment is
preferable, but does it need to be enforced? If so, then maybe the
`ELEMENT_LAYOUT` and example in the javadoc needs to be updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1372042718
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 13:08:06 GMT, Per Minborg wrote:
> This PR proposes removing the restriction that only heap `MemorySegment`
> wrapping a `byte` array can be accessed by Vectors. Now any array type can be
> used provided the element alignment constraints are respected.
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java
line 3284:
> 3282: * for any lane {@code N} in the vector
> 3283: * @throws IllegalArgumentException if the memory segment is a heap
> segment that is
> 3284: * not backed by a {@code byte[]} array and if access to
> the backing array
This seems backwards - e.g. a ByteVector requires an alignment of 1. So I would
expect to be able to pass _any_ segment?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1371810795
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 13:08:06 GMT, Per Minborg wrote:
> This PR proposes removing the restriction that only heap `MemorySegment`
> wrapping a `byte` array can be accessed by Vectors. Now any array type can be
> used provided the element alignment constraints are respected.
test/jdk/jdk/incubator/vector/AbstractVectorLoadStoreTest.java line 118:
> 116: private static boolean canBeConverted(IntFunction
> function, ValueLayout elementLayout) {
> 117: // Create a sample to analyze
> 118: MemorySegment s = function.apply(Long.BYTES);
I believe that a good way to test this is the following:
* each vector type operates as having a given element layout - for instance,
you can imagine the layout for IntVector to be `JAVA_INT` and so forth
* asking whether you load a segment into a vector is the same as asking whether
you can access the segment, at offset 0L, with the layout associated with the
vector (see above) - that is if MS::get throws, then vector load should also
throw (and viceversa)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1371817499
Re: RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
On Wed, 25 Oct 2023 13:08:06 GMT, Per Minborg wrote:
> This PR proposes removing the restriction that only heap `MemorySegment`
> wrapping a `byte` array can be accessed by Vectors. Now any array type can be
> used provided the element alignment constraints are respected.
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractVector.java
line 750:
> 748: AbstractMemorySegmentImpl requireSegmentConvertibleFor(MemorySegment
> segment, long offset, int elementByteSize) {
> 749: AbstractMemorySegmentImpl ams = (AbstractMemorySegmentImpl)
> segment;
> 750: if (ams.maxAlignMask() > 1 && !ams.isAlignedForElement(offset,
> elementByteSize)) {
I don't think we should only check for maxAlignMask > 1 - you also want to
check native segments, right?
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractVector.java
line 755:
> 753: .map(Class::componentType)
> 754: .map(Object::toString)
> 755: .orElse("?");
orElseThrow?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1371805656
PR Review Comment: https://git.openjdk.org/jdk/pull/16360#discussion_r1371806618
RFR: 8318678: Vector access on heap MemorySegments only works for byte[]
This PR proposes removing the restriction that only heap `MemorySegment` wrapping a `byte` array can be accessed by Vectors. Now any array type can be used provided the element alignment constraints are respected. - Commit messages: - Vector access on heap MemorySegments for all array types Changes: https://git.openjdk.org/jdk/pull/16360/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16360=00 Issue: https://bugs.openjdk.org/browse/JDK-8318678 Stats: 269 lines in 41 files changed: 126 ins; 23 del; 120 mod Patch: https://git.openjdk.org/jdk/pull/16360.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16360/head:pull/16360 PR: https://git.openjdk.org/jdk/pull/16360
