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

Fri Feb 9 15:36:05 UTC 2024

On Fri, 9 Feb 2024 14:05:31 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   small bugfix
>
> 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.

Renamed it with `extend_packset_with_more_pairs_by_following_use_and_def`

> 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()`

done.

> 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.

I can do that in a future RFE. I hope that the `_packset` and `_my_pack` eventually can become its own class. Then I can rename things everywhere.

> 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?

I want to keep them all there for now. I have some ideas about how to convert some of the filter methods to split methods. It is easier if everything is now a big laundry list, and we can group everything in a future RFE. Ok?

> 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?

done.

> 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.

done.

> 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()`?

Can I leave this for a future RFE?
[JDK-8325252](https://bugs.openjdk.org/browse/JDK-8325252) C2 SuperWord: refactor the packset

> 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`).

Is it ok if I leave that for a future RFE, where I refactor the `_packset` to a `PackSet` class?
[JDK-8325252](https://bugs.openjdk.org/browse/JDK-8325252) C2 SuperWord: refactor the packset

> 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?

Yes. And it does not make sense to print the packset and the filter_name for every round. That is why I have the printing at the end of all rounds below.

> 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()`?

done.

> 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.

I could. But I don't want to push this too far in this RFE. I could also make the whole function const.
Maybe I can have a future RFE where I just make everything const that can be const in SuperWord?
But also once I refactor out all the components (e.g. Reductions), then I can just make things const at that point.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484479678
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484481707
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484460336
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484457011
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484476877
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484475211
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484466156
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484463643
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484468435
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484471315
PR Review Comment: https://git.openjdk.org/jdk/pull/17785#discussion_r1484473226