RFR: 8325252: C2 SuperWord: refactor the packset [v4]

[Christian Hagedorn]

On Wed, 27 Mar 2024 13:58:17 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/superword.cpp line 1394:
>> 
>>> 1392: 
>>> 1393: SplitStatus PackSet::split_pack(const char* split_name,
>>> 1394:                                 Node_List* pack,
>> 
>> Just wondering if you also plan to have a separate `Pack` class at some point instead of using `Node_List`? If it's not worth we might still want to use a typedef to better show what the intent is. But that's something for another day.
>
> I will keep that in mind. It could really improve readability. Let's do it in a separate RFE!

Sounds good!

>> src/hotspot/share/opto/superword.cpp line 1539:
>> 
>>> 1537: 
>>> 1538: // Split packs at boundaries where left and right have different use or def packs.
>>> 1539: void SuperWord::split_packs_at_use_def_boundaries() {
>> 
>> As above with `PairSet`, I'm also wondering here if these `split` methods could be part of `PackSet`? But I have not checked all the method calls and it could very well be that you would need to pass a reference to `SuperWord` to `PackSet`. From a high-level view, "split packs" feels like an operation you perform on a pack set. Same, for example, as `SuperWord::verify_packs()`. 
>> 
>> Either way, I think the patch is already quite big and I think we should do that - if wanted - separately
>
> Yes, let's consider that for a future RFE.
> 
> But for now I can say this:
> We moved the implementation of `split / filter` to the packset, and that makes sense: we can hide the packset internals, and don't have them spill out to the SuperWord code. We can just call `_packset.split_packs` with a `split_strategy`. But defining the split strategy itself depends on other SuperWord components, and so I think conceptually they belong with SuperWord. They query `reductions`, `dependency graph`, `implemented`, `AlignmentSolution`, `profitable`, etc.
> Sure, we can just pass a SuperWord reference, but that does not really seem right to me either. For me, the SuperWord class is there to manage the interface between all the components, and try to avoid passing components to other components, wherever possible. So I would rather have a list of methods in SuperWord, and each such method defines how the components interact (e.g. packset and alignment, packset and AlignmentSolution, pairset and packset, ...).
> But we can discuss this further, and maybe come up with an even better solution. My hope is just that we separate the components as much as possible, so that we know that only a handful of them interact at a given point. That makes the whole beast more managable.

That's a valid point. Let's have another look at `SuperWord`, `PairSet` and `PackSet` once the code stabilizes to check whether it needs improvements or not by moving some code to the `*Set` classes.

>> src/hotspot/share/opto/superword.cpp line 2084:
>> 
>>> 2082:   // Create nodes (from packs and scalar-nodes), and add edges, based on the dependency graph.
>>> 2083:   void build() {
>>> 2084:     const PackSet& packset = _slp->packset();
>> 
>> You could also store the pack set as field since you access it several times.
>
> You are right, I could actually remove the SuperWord reference from the `PacksetGraph` completely, and just pass the components, as `const` references. That would be really nice.
> 
> If it is ok for you, I will do this in a future RFE.
> 
> Actually, I plan to completely overhaul the `PacksetGraph`. It will be transformed to the `VTransformGraph`, and it will do:
> - Cycle checking (like today)
> - Evaluate the cost-model
> - Execute: each node knows how to replace its packed scalar nodes with vector nodes (basically refactoring `SuperWord::output` away)
> - etc.
> 
> We may even be able to take the `VTransformGraph` and try to widen all nodes, or make transformations on this graph, a simplified version of IGVN. I have lots of ideas that would be unlocked with this new graph-based approach.

Sounds exciting! 

> If it is ok for you, I will do this in a future RFE.

Sure, let's leave this as it is now.

>> src/hotspot/share/opto/superword.cpp line 2404:
>> 
>>> 2402:   for (int i = 0; i < body().length(); i++) {
>>> 2403:     Node* n = body().at(i);
>>> 2404:     Node_List* p = _packset.pack(n);
>> 
>> Since you use this pattern a lot, you could also think about having a `SuperWord::pack()` method that delegates to `_packset.pack()`.
>
> Hmm. I tried it, but then we often also have a variable `pack`. So I now changed the `pack(n)` to `get_pack(n)`. I think that is better anyway, it suggests that we "get" something, rather than "pack" something.

Good point, `pack()` could be understood as query or command. `get_pack()` makes it clear 👍

>> src/hotspot/share/opto/superword.hpp line 69:
>> 
>>> 67:   // Doubly-linked pairs. If not linked: -1
>>> 68:   GrowableArray<int> _left_to_right; // bb_idx -> bb_idx
>>> 69:   GrowableArray<int> _right_to_left; // bb_idx -> bb_idx
>> 
>> I think it's a good solution but still found myself revisiting this several times while looking at the methods below how it works. Would it maybe help to give a visual example? For example:
>> 
>> 
>> left_to_right:
>>  index:    0   1    2    3
>>  value: | -1 | 3 | -1 | -1 | ...
>> 
>> => Node with bb_idx 1 is in the left slot of a pair which has the node with bb_idx 3 in the right slot.
>> => Nodes with bb_idx 0, 2, and 3 are not found in a left slot of any pair.
>>       
>> right_to_left:
>>  index:    0    1    2   3
>>  value: | -1 | -1 | -1 | 1 | ...
>> 
>> => Node with bb_idx 3 is in the right slot of a pair which has the node with bb_idx 1 in the left slot.
>> => Nodes with bb_idx 0, 1, and 2 are not found in a right slot of any pair.
>>  ```
>
> Great idea, I made a slightly more complex example, inspired by yours!

Cool, thanks for adding an even more extensive example. I like it and helps with understanding the idea better :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1541399343
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1541404378
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1541406166
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1541407489
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1541408431