RFR: 8333840: C2 SuperWord: wrong result for MulAddS2I when inputs permuted [v5]

[Christian Hagedorn]

On Thu, 13 Jun 2024 06:49:25 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> This is an older bug, I discovered it by manual code inspection while refactoring.
>> 
>> I added some regression tests that produced wrong results.
>> 
>> You should understand the fix given the comments in the code, if not: tell me how to improve them ;)
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits:
> 
>  - manual merge with master
>  - review changes for Vladimir
>  - more refactoring
>  - improve naming
>  - move code
>  - rm isa_pack_input_or_null, leave that to a refactoring
>  - refactoring / simplification
>  - refactor into is_muladds2i_pack_with_pack_inputs
>  - fix done, need refactoring
>  - more partial fix and more test
>  - ... and 3 more: https://git.openjdk.org/jdk/compare/5d2a19de...2155d18b

Small comments, otherwise, looks good!

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

> 2849: }
> 2850: 
> 2851: Node_List* PackSet::strided_pack_input_at_index_or_null(const Node_List* pack, const int index, const int stride, const int offset) const {

Do you plan to use this method at other places as well? Otherwise, you could hardcode `stride = 2` in the context of `MulAddS2I`.

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

> 2851: Node_List* PackSet::strided_pack_input_at_index_or_null(const Node_List* pack, const int index, const int stride, const int offset) const {
> 2852:   Node* p0 = pack->at(0);
> 2853:   Node* def0 = p0->in(index);

Can be merged:
Suggestion:

  Node* def0 = pack->at(0)->in(index);

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19619#pullrequestreview-2112955738
PR Review Comment: https://git.openjdk.org/jdk/pull/19619#discussion_r1638282834
PR Review Comment: https://git.openjdk.org/jdk/pull/19619#discussion_r1638275446