RFR: 8325520: Vector loads with offsets incorrectly compiled [v4]

[Damon Fenacci]

On Thu, 25 Apr 2024 12:39:32 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> // Load a float vector from the memory segment (internally, it does checkIndex and unsafe load from the byte array)
> FloatVector floatVector = FloatVector.fromMemorySegment(ms, offset, ByteOrder.nativeOrder())
> ```
> 
> I did not test this, but I think something like this should work.

Right! I totally missed that `from`- `intoMmemorySegment` methods! I guess that’s probably one reason why vector nodes are not typed.
I’ve added checks to `StoreNode::Identity` as well.

> src/hotspot/share/opto/memnode.cpp line 3533:
> 
>> 3531:       const Node* offsets = stv->in(StoreVectorScatterMaskedNode::Offsets);
>> 3532:       const Node* mask = stv->in(StoreVectorScatterMaskedNode::Mask);
>> 3533:       if (mem->is_StoreVectorScatterMasked()) {
> 
> This `if` will always be true, since we already check `mem->Opcode() == Opcode()`. The code would be simpler if you extracted the offsets and masks in parallel.

Yep, I removed this useless `if` and 2 other terms in the ifs before that. I'm just not sure of what you mean with
> if you extracted the offsets and masks in parallel.

> src/hotspot/share/opto/vectornode.hpp line 916:
> 
>> 914:   virtual int store_Opcode() const {
>> 915:     // Ensure it is different from any store opcode
>> 916:     return Op_LoadVectorGather;
> 
> I think you should take `-1`, which is what `MemNode::store_Opcode()` returns. It means "unknown".

OK. I was just a bit concerned that a check for equality between 2 `store_Opcode` could be true because they are both -1 but this shouldn’t happen. Changed.

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

PR Comment: https://git.openjdk.org/jdk/pull/18347#issuecomment-2095424211
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1590680121
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1590679021