RFR: 8347405: MergeStores with reverse bytes order value [v5]

[Emanuel Peter]

On Fri, 10 Jan 2025 10:52:58 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> kuaiwei has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - Fix build failure after merging
>>  - Merge remote-tracking branch 'origin/master' into pr/merge_stores_reverse
>>  - Exclude RISCV for Reverse nodes
>>  - Fix ppc64le
>>  - Update enum ValueOrder type
>>  - Fix as review comments
>>  - Remove unused test option
>>  - 8347405: MergeStores with reverse bytes order value
>
> src/hotspot/share/opto/memnode.cpp line 2956:
> 
>> 2954: }
>> 2955: 
>> 2956: bool MergePrimitiveStores::is_adjacent_input_pair(const Node* n1, const Node* n2, const int memory_size) {
> 
> Nit: This may be a little "nitpicky". But I don't like `is_...` methods that have side-effects. That is why I'd be sad to see the `const` go.

Ok, I would really like to have the `const` at the end of an `is_...` method.
I see that you want to take care of the value order.

Suggestion: instead of returning a `bool`, you could return an `enum`: `NotAdjacent`, `Forward` and `Backward`.

> test/hotspot/jtreg/compiler/c2/TestMergeStores.java line 803:
> 
>> 801:                    IRNode.REVERSE_BYTES_L, "1"},
>> 802:         applyIf = {"UseUnalignedAccesses", "true"},
>> 803:         applyIfPlatformAnd = {"little-endian", "true", "riscv64", "false"})   // Exclude RISV64 because ReverseBytesI/L are not supported
> 
> Can you please leave in the checks for the other stores (B, C, I)? I would like to have an exact count of each. This is so that we are sure that the others have disappeared as expected, not that some of them linger around and worsen performance.

Same in all the cases below

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23030#discussion_r1921905620
PR Review Comment: https://git.openjdk.org/jdk/pull/23030#discussion_r1921914594