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

[Roland Westrelin]

On Thu, 7 Mar 2024 06:53:21 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

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

Maybe. I think the current practice is to be extra careful and assume any input can be null during igvn. What do you think @vnkozlov ?

>> src/hotspot/share/opto/memnode.cpp line 3154:
>> 
>>> 3152:     }
>>> 3153:     ProjNode* other_proj = ctrl_s1->as_IfProj()->other_if_proj();
>>> 3154:     if (other_proj->is_uncommon_trap_proj(Deoptimization::Reason_range_check) == nullptr ||
>> 
>> This could be a range check for an unrelated array I suppose. Does it matter?
>
> I don't think it matters, no. Do you see a scenario where it would matter?
> 
> My argument:
> It is safe to do the stores after the RC rather than before it. And if the RC trap relies on the memory state of the stores that were before the RC, then those stores simply don't lose all their uses, and stay in the graph.
> After all, we only remove the "last" store by replacing it with the merged store, so the other stores only disappear if they have no other use.

Is there a chance then that we store to the same element twice (once with the store that we wanted to remove but haven't and the merged store)? I don't think repeated stores like this happen anywhere else as a result of some transformation. Would it be legal wrt the java specs? Can it be observed from some other thread? I think it would be better to not have to answer these questions and find a way to do the transformation in a way that guarantees the same element is not stored to twice.
Can the transformation be delayed until range check smearing has done its job?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1516355973
PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1516352600