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

[Damon Fenacci]

On Tue, 7 May 2024 12:42:38 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> 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
>
> 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?

I agree that it is quite convoluted probably also because I've put `if (!is_StoreVector())` (which is redundant) at the beginning to get the most common case out of the way but still...
At first I thought that multiple inheritance would be a good solution (masks and offsets could be inherited by the corresponding nodes) but the "HotSpot Coding Style" clearly says to avoid it...
So, I think in the end your second suggestion is the cleanest. Changing it...

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

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