On Thu, 12 Sep 2024 13:13:01 GMT, Roman Kennke <rken...@openjdk.org> wrote:
> > > > @rkennke Can you please explain the changes in these tests:
> > > > ```
> > > > test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java
> > > > test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationNotRun.java
> > > > test/hotspot/jtreg/compiler/loopopts/superword/TestAlignVector.java
> > > > test/hotspot/jtreg/compiler/loopopts/superword/TestIndependentPacksWithCyclicDependency.java
> > > > test/hotspot/jtreg/compiler/loopopts/superword/TestMulAddS2I.java
> > > > ```
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > You added these IR rule restriction: `@IR(applyIf =
> > > > {"UseCompactObjectHeaders", "false"},`
> > > > This means that if `UseCompactObjectHeaders` is enabled, vectorization
> > > > seems to be impacted - that could be concerning because it has a
> > > > performance impact.
> > > > I have recently changed a few things in SuperWord, so maybe some of
> > > > them can be removed, because they now vectorize anyway?
> > > > Of course some special tests may just rely on `UseCompactObjectHeaders
> > > > == false` - but I would need some comments in the tests where you added
> > > > it to justify why we add the restriction.
> > > > Please also test this patch with the cross combinations of
> > > > `UseCompactObjectHeaders` and `AlignVector` enabled and disabled (and
> > > > add `VerifyAlignVector` as well).
> > >
> > >
> > > IIRC (it has been a while), the problem is that with Lilliput (and also
> > > without compact headers, but disabling compressed class-pointers
> > > -UseCompressedClassPointers, but nobody ever does that), byte[] and
> > > long[] start at different offsets (12 and 16, respectively). That is
> > > because with compact headers, we are using the 4 bytes after the
> > > arraylength, but long-arrays cannot do that because of alignment
> > > constraints. The impact is that these tests don't work as expected,
> > > because vectorization triggers differently. I don't remember the details,
> > > TBH, but I believe they would now generate pre-loops, or some might even
> > > not vectorize at all. Those seemed to be use-cases that did not look very
> > > important, but I may be wrong. It would be nice to properly fix those
> > > tests, or make corresponding tests for compact headers, instead, or
> > > improve vectorization to better deal with the offset mismatch, if
> > > necessary/possible.
> > > I will re-evaluate those tests, and add comments or remove the
> > > restrictions.
> >
> >
> > If it has indeed been a while, then it might well be that some of them work
> > now, since I did make some improvements to auto-vectorization ;)
> > My suggestion is this: go over the examples, check which ones are now ok.
> > Those that are not ok, add a comment, and file a bug: I can then analyze
> > those cases later, and see how to write other tests or improve
> > auto-vectorization.
>
> Indeed, I could re-enable all tests in:
>
> ```
> test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java
> test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationNotRun.java
> test/hotspot/jtreg/compiler/loopopts/superword/TestIndependentPacksWithCyclicDependency.java
> ```
>
> but unfortunately not those others:
>
> ```
> > > > test/hotspot/jtreg/compiler/loopopts/superword/TestAlignVector.java
> > > > test/hotspot/jtreg/compiler/loopopts/superword/TestMulAddS2I.java
> ```
>
> I think the issue with all of them is that vectorization in those scenarios
> only works when the operations inside the loop start at an array index that
> addresses an element at 8-byte-aligned offset.
>
> I have filed https://bugs.openjdk.org/browse/JDK-8340010 to track it.
Excellent, that is what I hoped for! Thanks for filing the bug, I'll look into
it once this is integrated. You should probably mark it as "blocked by", not
"related to" ;)
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20677#issuecomment-2346266568
