RFR: 8325252: C2 SuperWord: refactor the packset

[Christian Hagedorn]

On Mon, 25 Mar 2024 11:06:24 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/superword.cpp line 1038:
>> 
>>> 1036: 
>>> 1037: // Extend pairset by following use->def and def->use links from pair members.
>>> 1038: void SuperWord::extend_pairset_with_more_pairs_by_following_use_and_def() {
>> 
>> Could this method (and possibly other methods only called in the context of this method) also be part of the new `PairSet` class?
>
> Not really. Inside we need lots of access to the SuperWord components (check `alignment`, `stmts_can_pack`, etc). You could always pass in a `SuperWord` reference, but that is not really nicer.
> Maybe it would be easier in the future, once I make some other changes. But we will always need to have other information available (dependency graph, types, etc - these are all VLoopAnalyzer submodules), but for now we also rely on `alignment`, which I plan to remove, and lives in SuperWord.

Passing a `SuperWord` reference is not wrong per se as `PairSet` is only used together with `SuperWord` and needs to have some information about it. A follow-up question arises if `stmts_can_pack()` and `find_adjacent_refs()` should then not also be part of `PairSet` together with this method. But anyway, since you plan to remove `alignment`, we could get back to this question there. Here, you only move/rename the code, so it's fine to leave it as it is now.

>> src/hotspot/share/opto/superword.cpp line 1232:
>> 
>>> 1230: //    use1->in(i) == def1 || use2->in(def2)   ->    use1->in(i) == def1 && use2->in(def2)
>>> 1231: //
>>> 1232: // 3: Add/Mul (use1, use2): we can try to swap edges:
>> 
>> Is it not required for other nodes?
>
> We can only do it for associative nodes like `Mul / Add`, and of course all the nodes that are subclasses of those (e.g. Max, Min, Or, etc). I can improve the comment, since that may not be immediately clear to the reader.

Thanks for the explanation. Yes, maybe you can clarify the comment :-)

>> src/hotspot/share/opto/superword.cpp line 1363:
>> 
>>> 1361:   for (PairSetIterator pair(_pairset); !pair.done(); pair.next()) {
>>> 1362:     Node* s1 = pair.left();
>>> 1363:     Node* s2 = pair.right();
>> 
>> Maybe you also want to name them `left` and `right` instead of `s1` and `s2`.
>
> Just specifically here, or everywhere in the code?

Good question, we could probably also try to apply this renaming at other places. If it's not too much work, you can try it.

>> src/hotspot/share/opto/superword.cpp line 1368:
>> 
>>> 1366:       pack->push(s1);
>>> 1367:     }
>>> 1368:     pack->push(s2);
>> 
>> We should also assert that at this point, `pack != nullptr`.
>
> Can do that, but I think it would just result in a nullptr exception anyway.
> I always wonder if an assert makes the code more cluttered, so less readable, or if it states expectations more explicitly, which makes the code more readable 😅

Right, well then you can leave it as is. One sometimes tends to be paranoid when it comes to using asserts in C2 :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1537466361
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1537467522
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1537468809
PR Review Comment: https://git.openjdk.org/jdk/pull/18276#discussion_r1537470307