RFR: 8310190: C2 SuperWord: AlignVector is broken, generates misaligned packs [v41]

Emanuel Peter epeter at openjdk.org
Tue Dec 12 18:27:55 UTC 2023


> I want to push this in JDK23.
> After this fix here, I'm doing [this refactoring](https://github.com/openjdk/jdk/pull/16620).
> 
> To calm your nerves: most of the changes are in auto-generated tests, and tests in general.
> 
> **Context**
> 
> `-XX:+AlignVector` ensures that SuperWord only creates LoadVector and StoreVector that can be memory aligned. This is achieved by iterating in the pre-loop until we reach the alignment boundary, then we can start the main loop properly aligned. However, this is not possible in all cases, sometimes some memory accesses cannot be guaranteed to be aligned, and we need to reject vectorization (at least partially, for some of the packs).
> 
> Alignment is split into two tasks:
>  - Alignment Correctness Checks: only relevant if `-XX:+AlignVector`. Need to reject vectorization if alignment is not possible. We must check if the address of the vector load/store is aligned with (divisible by) `ObjectAlignmentInBytes`.
>  - Alignment by adjusting pre-loop limit: alignment is desirable even if `-XX:-AlignVector`. We would like to align the vectors with their vector width.
> 
> **Problem**
> 
> I have recently found a bug with our AlignVector [JDK-8310190](https://bugs.openjdk.org/browse/JDK-8310190).
> In that bug, we perform a misaligned memory vector access, which results in a `SIGBUS` on an ARM32 machine.
> Thanks @fg1417 for confirming this!
> Hence, we need to fix the alignment correctness checks.
> 
> While working on this task, I also found some bugs in the "alignment by adjusting pre-loop limit": there were cases where it did not align the vectors correctly.
> 
> **Problem Details**
> 
> Reproducer:
> 
> 
>     static void test(short[] a, short[] b, short mask) {
>         for (int i = 0; i < RANGE; i+=8) {
>             // Problematic for AlignVector
>             b[i+0] = (short)(a[i+0] & mask); // best_memref, align 0
> 
>             b[i+3] = (short)(a[i+3] & mask); // pack at offset 6 bytes
>             b[i+4] = (short)(a[i+4] & mask);
>             b[i+5] = (short)(a[i+5] & mask);
>             b[i+6] = (short)(a[i+6] & mask);
>         }
>     }
> 
> 
> During `SuperWord::find_adjacent_refs` we used to check if the references are expected to be aligned. For that, we look at each "group" of references (eg all `LoadS`) and take the reference with the lowest offset. For that chosen reference, we check if it is alignable. If yes, we accept all references of that group, if no we reject all.
> 
> This is problematic as shown in this example. We have references at index offset `0, 3, 4, 5, 6`, and by...

Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 95 commits:

 - Merge branch 'master' into JDK-8311586
 - Refactoring: AlignmentSolution with nice subclasses (Christian's idea)
 - remove dead code
 - Merge branch 'master' into JDK-8311586
 - finished creating AlignmentSolver
 - v2 WIP AlignmentSolver
 - wip AlignmentSolver
 - aw -> alignment_vector in AlignmentSolution
 - more const
 - made invar const
 - ... and 85 more: https://git.openjdk.org/jdk/compare/df4ed7ef...690acf9a

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

Changes: https://git.openjdk.org/jdk/pull/14785/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14785&range=40
  Stats: 8448 lines in 23 files changed: 7157 ins; 376 del; 915 mod
  Patch: https://git.openjdk.org/jdk/pull/14785.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14785/head:pull/14785

PR: https://git.openjdk.org/jdk/pull/14785


More information about the hotspot-compiler-dev mailing list