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

[Roland Westrelin]

On Mon, 16 Dec 2024 06:52:23 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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.

Thanks for the explanation. I think it's reasonable to keep it in this change then.

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

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