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

[Emanuel Peter]

On Mon, 22 Jul 2024 13:47:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Fair enough. Maybe the code is ok as it is. Though the naming is quite bad, but that is not really your fault.
>> The more code we add the harder it will be to read.
>> So I would appreciate at least some more comments that help the reader.
>
> @eme64 what do you think of the refactoring & answers to your questions?

@rwestrel I'm sorry I didn't get back to this earlier.

It looks better, I like the `MemStoreQueue` and assert at the end of `already_enqueued`.

> > ```
> > * 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`).

Hmm. Personally I'd rather have something more vague than misleading. Maybe it could be `store -> use_mem_state` and `mem -> def_mem_state`? I guess that is then a bit wordy. Up to you.


sounds strange:
store->is_MergeMem()
store->is_Phi()

sounds more intuitive:
use_mem_state->is_MergeMem()
use_mem_state->is_Phi()


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

I understand, and agree on a technical level. Someone in the future may break things, and that is why I would prefer the assert to be there. But up to you.

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

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