RFR: 8318446: C2: optimize stores into primitive arrays by combining values into larger store [v17]

[Emanuel Peter]

On Wed, 6 Mar 2024 08:36:57 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   a little bit of casting for debug printing code
>
> src/hotspot/share/opto/memnode.cpp line 2802:
> 
>> 2800:   StoreNode* use = can_merge_primitive_array_store_with_use(phase, true);
>> 2801:   if (use != nullptr) {
>> 2802:     return nullptr;
> 
> Do you want to assert that the use is in the igvn worklist?

Hmm. I think that would not be a good assert.
Let's assume we have 4 stores that would merge. Then the last one of them does the merging, and replaces itself with the merged store. If `this` is the second store, then it could merge with its def (the first store). But since it has a use that could also be merged with, we delegate the merging down. But it is not done by the 3rd store, rather the 4th. So we cannot assert that the 3rd would be in the worklist. The 3rd may have been processed before, and determined that it does not want to idealize itself, and be removed from the worklist.

Maybe I can improve the comment:

// Merging is done by the last store in a chain. We have a use that could be merged with, so we
// are not the last store, and hence must wait for some (recursive) use to do the merge.

> src/hotspot/share/opto/memnode.cpp line 3146:
> 
>> 3144:   Node* ctrl_s1 = s1->in(MemNode::Control);
>> 3145:   Node* ctrl_s2 = s2->in(MemNode::Control);
>> 3146:   if (ctrl_s1 != ctrl_s2) {
> 
> Do you need to check that `ctrl_s1` and `ctrl_s2` are not null? I suppose this could be called on a dying part of the graph during igvn.

@rwestrel but then would they not be `TOP` rather than `nullptr`?

> src/hotspot/share/opto/memnode.hpp line 578:
> 
>> 576: 
>> 577:   Node* Ideal_merge_primitive_array_stores(PhaseGVN* phase);
>> 578:   StoreNode* can_merge_primitive_array_store_with_use(PhaseGVN* phase, bool check_def);
> 
> If I understand correctly you need the `check_def ` parameter to avoid having `can_merge_primitive_array_store_with_use` and `can_merge_primitive_array_store_with_def` call each other indefinitely. But if I was to write new code that takes advantage of one of the two methods, I think I would be puzzled that there's a `check_def` parameter. Passing `false` would be wrong then but maybe not immediately obvious. Maybe it would be better to have `can_merge_primitive_array_store_with_def` with no `check_def` parameter and have all the work done in a utility method that takes a `check_def` parameter (always `true` when called from `can_merge_primitive_array_store_with_def`)

You are right, this is not the best code pattern. I'll refactor it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1515627795
PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1515629804
PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1515628308