RFR: 8333393: PhaseCFG::insert_anti_dependences can fail to raise LCAs and to add necessary anti-dependence edges [v2]

[Daniel Lundén]

On Thu, 9 Jan 2025 12:40:10 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hmm, ok. I think the description here should be made more clear then, and explain what the strategy is, and attempt a kind of informal proof why this is an ok approach. What do you think?
>> 
>> I mean your comment says there are other cases, but it does not really reassure me that we have all cases covered 😅
>
> Another naming question:
> `other relevant initial memory states`
> What does the `initial` mean to you? To me, it is just the memory state of the load. `initial` only because we also look at other memory states.
> 
> If my definition is correct, then I wonder if talking about "other initial memory states" makes sense, or if they should have a different name for clarity. Maybe `root_memory_state` or alike. That would make sense: We have multiple trees, starting at `root_memory_state` each. `initial_mem` is one of them. `initial` only refers to things in the initial block, where the `initial_mem` is located.
> 
> Maybe you have a different definition - but it would be good to have a clear one stated in the comments.

> Hmm, ok. I think the description here should be made more clear then, and explain what the strategy is, and attempt a kind of informal proof why this is an ok approach. What do you think?

Do you mean a general proof sketch of the approach in `PhaseCFG::insert_anti_dependences`? This changeset does not really change the approach itself, but just extends the search to more initial memory states.

> but it does not really reassure me that we have all cases covered

I am not fully reassured we cover all cases either, but we at least now cover more cases than before (and specifically the cases that appear for this issue). See also the earlier comment by Roberto on investigating whether we can somehow enfore in earlier phases the invariant that the original version of `PhaseCFG::insert_anti_dependences` assumed: that we only need to search from one initial memory state. I would suggest doing this in a separate RFE.

> What does the initial mean to you?

To me it means the initial memory states that we start our searches at. I'm fine with your renaming suggestion, calling them roots. I would perhaps suggest that we then also say `input_mem` instead of `initial_mem` to signify that it is the actual input to the load.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22852#discussion_r1909152976