RFR: 8343685: C2 SuperWord: refactor VPointer with MemPointer [v3]

[Emanuel Peter]

On Fri, 13 Dec 2024 13:07:09 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 114 commits:
>> 
>>  - manual merge
>>  - fix printing
>>  - rename
>>  - fix up print
>>  - add TestEquivalentInvariants.java
>>  - improve documentation
>>  - hide parser via delegation
>>  - Merge branch 'master' into JDK-8343685-VPointer-MemPointer
>>  - make sort stable
>>  - some comment and naming improvements
>>  - ... and 104 more: https://git.openjdk.org/jdk/compare/31ceec7c...4b0504d0
>
> src/hotspot/share/opto/mempointer.cpp line 253:
> 
>> 251: // (2) LoadL from field jdk.internal.foreign.NativeMemorySegmentImpl.min
>> 252: //     Holds the address() of a native memory segment.
>> 253: bool MemPointerParser::is_native_memory_base_candidate(Node* n) {
> 
> Does this really belong to the move from VPointer to MemPointer? AFAIU, it's an extra optimization on top of the move. Should be done as a separate PR?

I had thought about splitting this out, but it I don't see a great way of doing that. Let me explain.

Before this patch, `MemPointer` parses through `CastX2P` in all cases, but `VPointer` never parses through `CastX2P`.

The current patch strikes a compromise:
Only parse through `CastX2P` if we have a `MemorySegment` access.

Let me explore some alternatives here:
- Do a separate patch, and make the change in `MemPointer` first, restricting parsing through `CastX2P` to MemorySegment cases. This could be done, but would not make sense on its own yet, as `MemPointer` is perfectly happy with parsing through `CastX2P` in all cases.
- First replace `VPointer` parsing with `MemPointer` logic. But then the question is what I do with the parsing through `CastX2P`.
  - Never parse through CastX2P: I would have to temporarily introduce some flag that is enabled for MergeStores, but disabled for VPointer. That is extra code you would have to review as well.
  - Always parse through CastX2P: that would make it impossible to find a base in non-MemorySegment cases of native memory access. There are some examples in 
  -  `test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java`. I would parse through `CastX2P`, and then it is not clear what would be a suitable "base", which I now need for `VPointer` (MergeStores did not need to know about bases).

TLDR: I'm not sure how to split this out without creating regressions, or creating unmotivated/untested code in the meantime. I also think that this code is relatively contained: it's all in `sub_expression_has_native_base_candidate` and `is_native_memory_base_candidate`.

I was also thinking of doing some refactoring of `mempointer.cpp/hpp` separately first - but again that would not be very well motivated: for example if I add the "Base" logic first separately, then it has no use. It will only be used by `VPointer`. And these changes are very well contained to those files, so if you just look at these files first, and only then at the other files it should be relatively straight forward.

Let me know if you have a good idea though.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1886266372