RFR: 8333258: C2: high memory usage in PhaseCFG::insert_anti_dependences() [v2]

[Roland Westrelin]

On Fri, 5 Jul 2024 07:53:52 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>     * The naming of `store` in this code is really bad. Rather than `mem` and `store`, it should be `use` and `def`. Because apparently a `store` can also be a `Phi` - that is not very intuitive.

I don't quite agree. I agree `store` is not great but people reading the code likely think about a `StoreNode` which is not always the case. But `use` is not better because it's too vague. It's not any kind of `use`. It's a `use` that also changing memory state (and in that respect it acts as a `store`).

>     * You removed the last:
> 
> 
> ```
> if (store->needs_anti_dependence_check())  continue;  // not really a store
> ```
> 
> That is fine, but I would replace it with an assert, this helps the reader to know that after the traversal loop we actually have some node with that characteristic.

The assert is on loop entry. There's a if between the new assert and the condition that was removed but the if block ends with a continue. So the assert is guaranteed to be executed every time the removed was executed.

>     * The argument you are making in `already_enqueued` (that all of `mem`'s uses are processed in one go) is quite implicit. How much runtime would we actually lose if we scanned the whole worklist instead? I'm just wondering if this is not a premature optimization, that then requires an argument that is not clear to the reader (it was not immediately clear to me and also not to Vladimir K).

I could add debug code that goes over the rest of the list and checks it doesn't find that pair enqueued elsewhere.

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

PR Comment: https://git.openjdk.org/jdk/pull/19791#issuecomment-2211066901