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

[Tobias Hartmann]

On Tue, 21 May 2024 13:47:32 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue
>> When loading multiple vectors using offsets or masks (e.g. `LongVector::fromArray(LongVector.SPECIES_256, storage, 0, offsets, 0` or `LongVector::fromArray(LongVector.SPECIES_256, storage, 0, longMask)`) there is an error in the C2 compiled code that makes different vectors be treated as equal even though they are not.
>> 
>> # Causes
>> On vector-capable platforms, vector loads with masks and offsets (for Long, Integer, Float and Double) create specific nodes in the ideal graph (i.e. `LoadVectorGather`, `LoadVectorMasked`, `LoadVectorGatherMasked`). Vector loads without mask or offsets are mapped as `LoadVector` nodes instead.
>> The same is true for `StoreVector`s.
>> When running GVN loops we can get to the situation where we check if a Load node is preceded by a Store of the same address to be able to replace the Load with the input of the Store (in `LoadNode::Identity`). Here we call
>> 
>> https://github.com/openjdk/jdk/blob/87e864bf21d71daae4e001ec4edbb4ef1f60c36d/src/hotspot/share/opto/memnode.cpp#L1258
>> 
>> where we do an extra check for types if we deal with vectors but we don’t make sure that neither masks nor offsets interfere.
>> Similarly, in `StoreNode::Identity`  we first check if there is a Load and then a Store:
>> 
>> https://github.com/openjdk/jdk/blob/87e864bf21d71daae4e001ec4edbb4ef1f60c36d/src/hotspot/share/opto/memnode.cpp#L3509-L3515
>> 
>> but we don’t make sure that there are no masks or offsets. 
>> A few lines below, we check if there are 2 stores for the same value in a row. We need to check for masks and offsets here too but in this case we can include these cases if the masks and offsets of the vector stores are equivalent.
>> 
>> # Solution
>> To avoid folding `Load`- and `StoreVector`s with masks and offsets we add a specific `store_Opcode` method to `LoadVectorGatherNode`, `LoadVectorMaskedNode` and `LoadVectorGatherMaskedNode` that doesn’t return a store opcode but instead returns -1. In this way, the checks in `MemNode::can_see_stored_value`
>> 
>> https://github.com/openjdk/jdk/blob/87e864bf21d71daae4e001ec4edbb4ef1f60c36d/src/hotspot/share/opto/memnode.cpp#L1164-L1166
>> 
>> and `StoreNode::Identity`
>> 
>> https://github.com/openjdk/jdk/blob/87e864bf21d71daae4e001ec4edbb4ef1f60c36d/src/hotspot/share/opto/memnode.cpp#L3509-L3515
>> 
>> will fail if masks or offsets are used.
>> For 2 stores of the same value we instead check for mask and offset equality.
>> 
>> Regression tests for all versions of `Load/StoreVectorGather/Masked` hav...
>
> Damon Fenacci has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8325520: add override keyword

Nice regression test! I added a few comments.

src/hotspot/share/opto/vectornode.hpp line 893:

> 891: class LoadVectorGatherNode : public LoadVectorNode {
> 892:  public:
> 893:   enum { Offsets = 3 };

Just wondering about the naming convention here because the corresponding constructor argument is called `indices`. Should that be consistent?

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 -1;

Maybe improve this comment and explain that we are doing this to avoid folding which does not account for the mask/offsets. Same for the comments in the other `store_Opcode` methods.

src/hotspot/share/opto/vectornode.hpp line 1007:

> 1005: class LoadVectorMaskedNode : public LoadVectorNode {
> 1006:  public:
> 1007:   enum { Mask = 3 };

Where is this used?

src/hotspot/share/opto/vectornode.hpp line 1034:

> 1032:   enum { Offsets = 3,
> 1033:          Mask
> 1034:   };

Where is this used?

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

Changes requested by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18347#pullrequestreview-2070390543
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1609457204
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1609467759
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1609470581
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1609470073