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

[Emanuel Peter]

On Mon, 6 May 2024 15:20: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 its own (to avoid ever being the same as a store node). 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...
>
> Damon Fenacci has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - JDK-8325520: add store/load masked vector tests
>  - JDK-8325520: add store/load tests with duplicate offsets

Nice, looks much better, I think the VM code is now correct.
A few suggestions for code style.

src/hotspot/share/opto/memnode.cpp line 1169:

> 1167:       // LoadVector/StoreVector need additional checks
> 1168:       if (st->is_StoreVector()) {
> 1169:         // Ensure that types match

To reduce  noise, you could revert these comment changes, up to you.

src/hotspot/share/opto/memnode.cpp line 3518:

> 3516:       mem->in(MemNode::ValueIn)->eqv_uncast(val) &&
> 3517:       mem->Opcode() == Opcode()) {
> 3518:     // Not a vector

Suggestion:


Redundant comment, the next line literally says as much ;)

src/hotspot/share/opto/memnode.cpp line 3546:

> 3544:           const StoreVectorScatterMaskedNode* svgm = mem->as_StoreVectorScatterMasked();
> 3545:           if (offsets->eqv_uncast(svgm->in(StoreVectorScatterMaskedNode::Offsets)) &&
> 3546:             mask->eqv_uncast(svgm->in(StoreVectorScatterMaskedNode::Mask))) {

Suggestion:

              mask->eqv_uncast(svgm->in(StoreVectorScatterMaskedNode::Mask))) {

src/hotspot/share/opto/memnode.cpp line 3551:

> 3549:         // Regular store (no offsets or mask)
> 3550:         } else {
> 3551:           result = mem;

Suggestion:

          assert(Opcode() = Op_StoreVector, "just a plain vector store, no offset or mask");
          result = mem;

Turning comments into asserts is preferable I would say.

src/hotspot/share/opto/memnode.cpp line 3554:

> 3552:         }
> 3553:       }
> 3554:     }

I think the code is now correct.
But I find the nested if-elseif-elseif-else ... structure a bit hard to read. And there is quite some code duplication (e.g. `result = mem` and all the `eqv_uncast` checks).

You could either do something like this:

if (!is_StoreVector() ||
    as_StoreVector()->has_same_vect_type_and_offsets_and_mask(mem->as_StoreVector())) {
  result = mem;
}


Sketch:

has_same_vect_type_and_offsets_and_mask:

different vect_type -> return false
...


Or maybe it would be better to define virtual functions to get the `mask` and `offsets` from a `StoreVector`? If it has none, just return `nullptr`. Sometimes people worry about virtual methods, but we already use them extensively for the node Value/Ideal anyway.

Then, you can do:

if (!is_StoreVector()) {
  result = mem;
} else {
  const Node* offsets1 = as_StoreVector()->get_offsets();
  const Node* offsets2 = mem->as_StoreVector()->get_offsets();
  const Node* mask1 = as_StoreVector()->get_mask();
  const Node* mask2 = mem->as_StoreVector()->get_mask();
  if (offsets1->eqv_uncast(offsets2) && offsets1->eqv_uncast(offsets2)) {
    result = mem;
  }
}

I think that would be the cleanest and most readable way.

What do you think?

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

PR Review: https://git.openjdk.org/jdk/pull/18347#pullrequestreview-2043033219
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1592390146
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1592392528
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1592399071
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1592396514
PR Review Comment: https://git.openjdk.org/jdk/pull/18347#discussion_r1592417634