RFR: 8325541: C2 SuperWord: refactor filter / split

[Emanuel Peter]

On Fri, 9 Feb 2024 10:06:59 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> A few filter / split methods are currently bunched together. We should peal them apart so that moving / changing things becomes easier in the future.
> 
> 
> combine_packs -> refactored into:
>   combine_packs (now really only combines)
>   split_packs_for_max_vector_size
>   filter_packs_for_power_of_2_size
>   filter_packs_for_mutual_independence
> 
> filter_packs -> refactored into:
>   filter_packs_for_implemented
>   filter_packs_for_profitable
> 
> 
> Since we have so many "filter" passes, I created a dedicated `SuperWord::filter_packs` method, that takes a `FilterPredicate` lambda. This also allows me now to systematically debug dump any packs that are rejected .
> 
> I also took the liberty to enforce a new invariant between passes: there cannot be any `nullptr` in the packlist.
> Before the "filter" passes just set some packs to `nullptr`. I now shift all surviving packs down, and truncate the packlist.

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

> 1570:       assert(is_power_of_2(max_vlen), "sanity");
> 1571:       uint psize = p1->size();
> 1572:       if (!is_power_of_2(psize)) {

Note: this is now in `filter_packs_for_power_of_2_size`

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

> 1571: 
> 1572:   // Remove all nullptr from packset
> 1573:   compress_packset();

Note: I will refactor `combine_packs` so that it does not need this in the future. But for now the "combine" code does introduce `nullptr`, and I want to remove them here, to establish the new invariant: no `nullptr` in packlist.

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

> 1582:         continue;
> 1583:       }
> 1584:       if (psize > max_vlen) {

Note: this is now in `split_packs_for_max_vector_size`

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

> 1616:   // other hand, "read forward" or "store backward" cases do not have such dependencies:
> 1617:   // for (int i ...) { v[i] = v[i + 1] + 5; }
> 1618:   // for (int i ...) { v[i - 1] = v[i] + 5; }

Note: I moved this to `filter_packs_for_mutual_independence`, and changed indentation.

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

> 1622:       // reductions are trivially connected
> 1623:       if (!is_marked_reduction(p->at(0)) &&
> 1624:           !mutually_independent(p)) {

Note: this is the condition in `filter_packs_for_mutual_independence`

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

> 1644:         tty->cr();
> 1645:         tty->print_cr("WARNING: Removed pack: %s:", error_message);
> 1646:         print_pack(pack);

Note: bonus: all packs that are filtered out are now logged as rejections.

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

> 1694:   for (int i = 0; i < _packset.length(); i++) {
> 1695:     Node_List* p = _packset.at(i);
> 1696:     if (p != nullptr) {

Note: mostly change due to indentation.
Rename `p -> pack`
And added the `new_packset_length`, `keep` logic, instead of
`_packset.at_put(i, nullptr);` and then later removing all `nullptr` with `compress_packset`.

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

> 1718:           }
> 1719: #endif
> 1720:           _packset.at_put(i, nullptr);

Note: I now shift the surviving (keep) packs down, rather than setting `nullptr` and `compress_packset` below.

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

> 1745: 
> 1746:   // Remove all nullptr from packset
> 1747:   compress_packset();

Note: not necessary because I now shift instead of setting `nullptr`

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

> 1795:   for (int i = _packset.length() - 1; i >= 0; i--) {
> 1796:     Node_List* pk = _packset.at(i);
> 1797:     bool impl = implemented(pk);

Note: now in `filter_packs_for_implemented`

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

> 1819:     for (int i = _packset.length() - 1; i >= 0; i--) {
> 1820:       Node_List* pk = _packset.at(i);
> 1821:       bool prof = profitable(pk);

Note: now in `filter_packs_for_profitable`

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

> 3435:     set_my_pack(s, nullptr);
> 3436:   }
> 3437:   _packset.at_put(pos, nullptr);

Note: shifting happens at all call-sites. I just set `nullptr` to be sure the pack does not survive on the packset.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484137890
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484129245
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484137540
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484136186
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484136553
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484139840
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484125259
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484127546
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484127821
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484140712
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484141131
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484142485