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

[Damon Fenacci]

On Tue, 23 Apr 2024 09:50:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Damon Fenacci has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - JDK-8325520: take advantage of store_Opcode to avoid more checks
>>  - JDK-8325520: remove check for offsets/mask equivalence if load->store or store->load
>>  - JDK-8325520: merge
>>  - JDK-8325520: fix missing semicolon
>>  - JDK-8325520: address PR comments
>
> src/hotspot/share/opto/memnode.cpp line 2830:
> 
>> 2828:       val->in(MemNode::Memory )->eqv_uncast(mem) &&
>> 2829:       val->as_Load()->store_Opcode() == Opcode()) {
>> 2830:         // Handle StoreVector with offsets and masks
> 
> Also: the indendation is not right: it should only be 2 spaces from the `if`.

Yep, fixed, thanks.

> src/hotspot/share/opto/memnode.cpp line 2858:
> 
>> 2856:         } else {
>> 2857:           result = mem;
>> 2858:         }
> 
> You already have the condition `val->as_Load()->store_Opcode() == Opcode()`.
> So once you have `is_StoreVectorScatter()`, I think `val->is_LoadVectorGather()` is implied, no?
> 
> Oh wow. I think actually that this is another bug here: we only have
> `virtual int store_Opcode() const { return Op_StoreVector; }` for `LoadVectorNode`, but not for all masked/gather/scatter vector nodes! I think that should be fixed.
> 
> That would also simplify your code.

I’ve updated the `store_Opcode` methods. Actually I’ve used them to always avoid being the same as a Store opcode (I’ve used the Load own opcodes (a bit hacky) but that can be changed). See the comment below.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1579069186
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1579068284