RFR: 8340010: Fix vectorization tests with compact headers

[Emanuel Peter]

On Thu, 21 Nov 2024 10:06:13 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> This is a followup to:
>> https://github.com/openjdk/jdk/pull/20677 / [JDK-8305895](https://bugs.openjdk.org/browse/JDK-8305895) Implement JEP 450: Compact Object Headers (Experimental)
>> 
>> @rkennke fixed the vectorization tests in a very rudamentary way. I now took some time to see what exactly is affected. At the time I reviewed the JEP, I thought only very minor cases were affected, like hand-unrolling etc. But it turns out that there are some more important cases that are affected, like **mixed-type loops**, such as **conversion** between types. Another class of affected loops is **hand-unrolled loops**.
>> 
>> **The problem is this:**
>> Since the offset from array-base to payload has changed (16 -> 12), some vector loads/stores can now no longer be aligned. This means vectorization under `+AlignVector` is not possible.
>> 
>> --------------------------------
>> 
>> First: only platforms that require strict-alignment are affected (i.e. `+AlignVector` or `Matcher::misaligned_vectors_ok=false`). I filed [JDK-8344424](https://bugs.openjdk.org/browse/JDK-8344424) for this, so the impact can be discussed. **The affected platforms seem to be exceptions**.
>> 
>> -------------------
>> 
>> Now on to fixing the tests, which we need to do now anyway. Some actually were currently failing.
>> 
>> I once ran over `tier1,tier2,tier3,tier4` plus our internal stress testing with `+-AlignVector` and `+-UseCompactObjectHeaders`, collected all failing tests. I also looked at all tests that were already guarding IR rules on `UseCompactObjectHeaders`.
>> 
>> To almost all tests I added runs with `+-AlignVector` and `+-UseCompactObjectHeaders`. We could leave this also to global runs with these flag combinations. But it is rare that we ever run this, so I thought I want to directly run the "interesting" tests with all combinations. This requires extra test runtime, but I think it is warranted.
>> 
>> In a few cases I also added stronger IR rules, in tests that were already affected - just to make sure we have the behavior we want. Some cases would not vectorize for other cases, and I put comments there for future reference.
>
> test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java line 184:
> 
>> 182:     @IR(counts = { IRNode.LOAD_VECTOR_L, ">=1", IRNode.STORE_VECTOR, ">=1" },
>> 183:         // This test fails with compact headers, but only with UseSSE<=3.
>> 184:         applyIf = { "UseCompactObjectHeaders", "false" },
> 
> Is this removed intentiionally? or it should be `applyIfOr = {"UseCompactObjectHeaders", "false", "AlignVector", "false"},`?

Well, it turns out that this case always vectorizes. Did you see the comments below? It is a strange exception, because this test manually sets a large "base-to-payload-offset" of 64.

> test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java line 196:
> 
>> 194:     @IR(counts = { IRNode.LOAD_VECTOR_L, ">=1", IRNode.STORE_VECTOR, ">=1" },
>> 195:         // This test fails with compact headers, but only with UseSSE<=3.
>> 196:         applyIf = { "UseCompactObjectHeaders", "false" },
> 
> same as above comment.

And same response as above ;)

> test/hotspot/jtreg/compiler/vectorization/runner/ArrayTypeConvertTest.java line 111:
> 
>> 109:     // ---------------- Integer Extension ----------------
>> 110:     @Test
>> 111:     @IR(failOn = {IRNode.STORE_VECTOR})
> 
> Not sure if it's necessary to fail on the Node, I'm not an expert, so just a question, not a review comment. Similar question for the below  `failOn`s

I think it makes sense to add a `failOn` here. We have an RFE that tracks this, and when it is implemented, we should expect this test to "fail", i.e. the vectorization succeeds. Then it would be nice if we can enable a positive IR rule. So I put in a negative now, so we don't forget to put in a positive later.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22199#discussion_r1851786764
PR Review Comment: https://git.openjdk.org/jdk/pull/22199#discussion_r1851787075
PR Review Comment: https://git.openjdk.org/jdk/pull/22199#discussion_r1851783591