RFR: 8325541: C2 SuperWord: refactor filter / split [v2]

Fri Feb 9 15:09:05 UTC 2024

On Fri, 9 Feb 2024 14:10:17 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.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   small bugfix

Neat refactoring and nice usage of lambdas! I have some comments.

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

> 571:   }
> 572: 
> 573:   extend_packlist();

Maybe we should also rename this at some point to better describe what `extend` actually means and that we are only adding new pairs and not other sized packs.

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

> 574: 
> 575:   // Combine pairs to longer packs
> 576:   combine_packs();

How about also naming the method `combine_pairs()` or `combine_pairs_to_longer_packs()`

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

> 580: 
> 581:   // Now we only remove packs:
> 582:   construct_my_pack_map();

`construct_my_pack_map()` also asserts that each node is only in one pack after `combine_packs()`. Maybe we should move this code directly to `combine_packs()` as a post-condition. 

This was already like this before, but it seems like "my" in `_my_pack` in `SWNodeInfo` is redundant and could just be renamed to `_pack`. Then we could also rename this method to something like `map_nodes_to_packs()`. But this exceeds the scope of this PR and could also be done separately if you agree with this suggestion.

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

> 585:   filter_packs_for_alignment();
> 586:   filter_packs_for_implemented();
> 587:   filter_packs_for_profitable();

How about keeping `filter_packs()` which then calls all 5 new `filter_packs*` methods?

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

> 1541: // Combine packs A and B with A.last == B.first into A.first..,A.last,B.second,..B.last
> 1542: void SuperWord::combine_packs() {
> 1543:   assert(!_packset.is_empty(), "packset not empty");

Could be moved into the assert block below for completeness.

Should we also assert, that we only have packs that are pairs at this point?

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

> 1626:   }
> 1627: 
> 1628:   assert(!_packset.is_empty(), "we only increased the number of packs");

You could also store the number of packs on methods entry and now check that the number of packs is greater or equal.

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

> 1653:         tty->cr();
> 1654:         tty->print_cr("WARNING: Removed pack: %s:", error_message);
> 1655:         print_pack(pack);

Could this be part of `remove_pack_at()`?

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

> 1790:     if (keep) {
> 1791:       assert(i >= new_packset_length, "only move packs down");
> 1792:       _packset.at_put(new_packset_length++, pack);

Since you use this pattern in multiple places now, how about having a `PackSetRemover` that handles iteration and safe removal without requiring the user to keep track of the length and doing the truncation itself? We could for example execute the truncation with `trunc_to` in the destructor (if we ensure that it is a `StackObj`).

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

> 1883:   while (true) {
> 1884:     int old_packset_length = _packset.length();
> 1885:     filter_packs(nullptr, // don't dump each time

Is it too verbose?

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

> 512: 
> 513:   // Split packs that are too long
> 514:   void split_packs_for_max_vector_size();

How about naming this `split_packs_longer_than_max_vector_size()`?

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

> 546:   Node* vector_opd(Node_List* p, int opd_idx);
> 547:   // Can code be generated for pack p?
> 548:   bool implemented(const Node_List* p);

Could be made `const` as well if you also make `is_marked_reduction()` const.

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

PR Review: https://git.openjdk.org/jdk/pull/17785#pullrequestreview-1872478962
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484364700
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484361759
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484388518
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484365725
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484366634
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484402159
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484411800
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484433152
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484436356
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484440011
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484443041