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

[Damon Fenacci]

On Tue, 7 May 2024 15:29:11 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

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

I've updated it. The condition unfortunately doesn't look as clean as the one above as we need to check for `nullptr` (either both or none and `eqv_uncast`). I've tried to make it as concise as possible (we could have made `mask` and `offsets` return a _unique_ node instead, so as to avoid the `nullptr`, but I had the impression it would just make everything less clear).

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

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