RFR: 8326139: C2 SuperWord: split packs (match use/def packs, implemented, mutual independence) [v7]

[Christian Hagedorn]

On Mon, 11 Mar 2024 07:36:11 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> After `combine_pairs_to_longer_packs`, we sometimes create packs that are too long and cannot be vectorized.
>> There are multiple reason for that:
>> 
>> - A pack does not "match" with a use of def pack, and we need to split it. Example: split Z:
>> 
>> X X X X Y Y Y Y
>> Z Z Z Z Z Z Z Z
>> 
>> 
>> - A pack is not implemented in the current size, but would be implemented for a smaller size. Some operations are not implemented at max vector size, and we just need to split every pack to the smaller size that is implemented. Or we have a pack of a non-power-of-2 size, and need to split it down into smaller sizes that are power-of-2.
>> 
>> - Packs can have pack internal dependence. This dependence happens at a certain "distance". If we split a pack to be smaller than that "distance", then the internal dependence disappears, and we have the desired mutual independence. Example:
>> https://github.com/openjdk/jdk/blob/9ee274b0480a6e8e399830fd40c34d99c5621c1b/test/hotspot/jtreg/compiler/loopopts/superword/TestSplitPacks.java#L657-L669
>> 
>> Note: Soon I will refactor the packset into a new `PackSet` class, and move the split / filter code there.
>> 
>> **Further Work**
>> 
>> [JDK-8309267](https://bugs.openjdk.org/browse/JDK-8309267) C2 SuperWord: some tests fail on KNL machines - fail to vectorize
>> The issue on KNL machines is that some operations only support vector widths that are smaller than the maximal vector length. Hence, we must split all other vectors that are uses/defs. This change here does exactly that, and so I will be able to put the `UseKNLSetting` in the IRFramework whitelist. I will do that in a separate RFE.
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 34 commits:
> 
>  - manual merge master
>  - Merge branch 'master' into JDK-8309267
>  - Apply suggestions for comments by Vladimir
>  - Update LoopArrayIndexComputeTest.java copyright year
>  - Update src/hotspot/share/opto/superword.cpp
>  - SplitStatus::Kind enum
>  - SplitTask::Kind enum
>  - manual merge
>  - more fixes for TestSplitPacks.java
>  - fix some IR rules in TestSplitPacks.java
>  - ... and 24 more: https://git.openjdk.org/jdk/compare/ca5ca85d...efab8718

Apart from some minor comments, looks good to me, too!

src/hotspot/share/opto/superword.cpp line 1582:

> 1580: }
> 1581: 
> 1582: // Split packs that have mutual dependence, until all packs are mutually_independent.

Suggestion:

// Split packs that have a mutual dependency, until all packs are mutually_independent.

src/hotspot/share/opto/superword.cpp line 1590:

> 1588:                  if (!is_marked_reduction(pack->at(0)) &&
> 1589:                      !mutually_independent(pack)) {
> 1590:                    // Split in half.

Maybe you could add a comment here that splitting in half is a best guess/intuitive way to continue

src/hotspot/share/opto/superword.cpp line 3017:

> 3015:     if (!is_reduction_pack &&
> 3016:         (!has_use_pack_superset(n0, n1) ||
> 3017:          !has_use_pack_superset(n1, n0))) {

Was first tricked by missing the inversion of the result. Maybe you can flip it and rename it to `has_no_use_pack_superset()`?

src/hotspot/share/opto/superword.hpp line 339:

> 337:     const char* message() const { return _message; }
> 338: 
> 339:     int split_size() const {

Should be `uint`:
Suggestion:

    uint split_size() const {

src/hotspot/share/opto/superword.hpp line 393:

> 391:   void split_packs(const char* split_name, SplitStrategy strategy);
> 392: 
> 393:   // Split packs at boundaries where left and right have different use or def packs.

Just a general note, I'm not sure if you need to repeat the comments here when they are identical to the ones found at the definition in the source file. But I guess it does not hurt either. If you only want to keep one, I'd prefer to have the comments in the source file.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17848#pullrequestreview-1927154552
PR Review Comment: https://git.openjdk.org/jdk/pull/17848#discussion_r1519309037
PR Review Comment: https://git.openjdk.org/jdk/pull/17848#discussion_r1519459587
PR Review Comment: https://git.openjdk.org/jdk/pull/17848#discussion_r1519294500
PR Review Comment: https://git.openjdk.org/jdk/pull/17848#discussion_r1519296397
PR Review Comment: https://git.openjdk.org/jdk/pull/17848#discussion_r1519306208