RFR: 8325520: Vector loads with offsets incorrectly compiled

[Damon Fenacci]

On Thu, 21 Mar 2024 08:46:27 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> Can you please fix the whitespace issues, it is a bit difficult to read now on GitHub ;)

Sorry, I let them slip in. Fixed.

> I think you checks should also not be done **after** we already create a new node, but ideally before we create the new node. That is the normal pattern I see everywhere. So then you would probably have to dig into `MemNode::can_see_stored_value` and other places, to see where you would need to do your additional checks. There are already some checks for vector-type there, so that would be one intuitive starting-point.

I've removed the `Identity` overrides and changed the checks in `MemNode::can_see_stored_value` as you suggested (`Identity` methods were also a bit too restrictive). I also had to add checks to `StoreNode::Identity` here

https://github.com/openjdk/jdk/blob/7eb78e332080df3890b66ca04338a4ba69af45eb/src/hotspot/share/opto/memnode.cpp#L2799-L2814

> src/hotspot/share/opto/vectornode.cpp line 1148:
> 
>> 1146: Node* StoreVectorMaskedNode::Identity(PhaseGVN* phase) {
>> 1147:   Node* value = StoreVectorNode::Identity(phase);
>> 1148:   if ((value != this) &&  (value->is_StoreVectorMasked()) && (in(MemNode::ValueIn + 1)->eqv_uncast(value->in(MemNode::ValueIn + 1)))) {
> 
> `MemNode::ValueIn + 1` is there no direct value for it? If not: maybe create one?

Thanks for the comments @eme64. 
I've created one more constant for the "+ 2" case (`OffsetsMask`) and reused the one there for the "+ 1" case (`OopStore`).

> test/hotspot/jtreg/compiler/vectorapi/VectorGatherMaskFoldingTest.java line 43:
> 
>> 41:  * @modules jdk.incubator.vector
>> 42:  *
>> 43:  * @run main/othervm -XX:UseAVX=3 compiler.vectorapi.VectorLoadGatherFoldingTest
> 
> I would also make sure that you don't require platform specific things here. This could also run on any other platform that supports the Vector API, right? And you will need at least one run without any flags.

You're right. I've relaxed the require (the max vector size shouldn't be relevant) and added `sve`. I'm not sure if there is a way to make it more generic (all platforms that support vectors?).

> test/hotspot/jtreg/compiler/vectorapi/VectorGatherMaskFoldingTest.java line 46:
> 
>> 44:  */
>> 45: 
>> 46: public class VectorLoadGatherFoldingTest {
> 
> I assume you will also do a similar test for `Store`?
> It would for example be nice to see IR tests that verify that some Stores/Loads are folded together (where it is ok), and others are not folded, because they have different masks.
> Would it make sense to make this a IR test?

Yep, there are actually `Store` tests already but I forgot to adapt the name. Fixed now.
I've tried to use the IR framework to check for folded/non folded nodes but couldn't make it reliable enough (in the end it didn't test more than the current test). So I decided to go back to the actual "regression" test which reproduces the original issue.

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

PR Comment: https://git.openjdk.org/jdk/pull/18347#issuecomment-2011750958
PR Comment: https://git.openjdk.org/jdk/pull/18347#issuecomment-2044298493
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1540681271
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1540682167
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1540681799