RFR: 8333334: C2: Make result of `Node::dominates` more precise to enhance scalar replacement [v5]

[Qizheng Xing]

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

>> Qizheng Xing has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add `@requires` for G1 GC.
>
> Nice, looks like this is a worth-wile improvement!
> 
> I have some comments about style and software-engineering. Mainly:
> - Generally return values passed via a argument is already a bit of a nasty solution - but sometimes it is the least nasty solution - so fine.
> - But if you do pass a return value via argument, it should probably be a reference, not a pointer. But yours is not just a pointer, but an optional pointer.
> - That leads you to the `dummy_flag` hack.
> - How about this alternative: You create two `MemNode::all_controls_dominate` methods, one without the `bool& dead_code`, and one with. The one without simply creates a local variable and delegates to the implementation with the return-argument. Would that work?
> - I would also give `dead_code` a more descriptive name. Currently it is not immediately clear what this flag is. For one, you should probably mention above `MemNode::all_controls_dominate` that it is a return-argument. Maybe call it `encountered_dead_code`.
> 
> Also: there are only 11 occurances of `all_controls_dominate`. Maybe you could have a `enum` return instead of a `bool`. It could have states `Dominate`, `Not_Dominate` and `Encountered_Dead_Code`. You would of course have to adjust all use-cases accordingly - but maybe we want to look at all use-cases anyway and make sure we handle the dead-code case correctly.
> 
> Side note:
> `bool dummy_flag, &dead_code_flag = dead_code != nullptr ? *dead_code : dummy_flag;`
> 
> Lines like this make me really nervous. I'm not sure if the `dummy_flag` is initialized (maybe it doesn't matter, but still).
> 
> I'm not settled on any particular solution here, these are just my first reactions ;)
> 
> What do you think?

@eme64 Thanks for your patience and detailed comments!

I agree with you that returning values via arguments, especially pointer arguments, is not a good idea. I think returning a enum value would be an acceptable solution, so I've made the changes for methods `Node::dominates` and `MemNode::all_controls_dominate` to return `enum class DomResult` with values `Dominate`, `NotDominate` and `EncounteredDeadCode` as you suggested.

This solution does not contain any dummy flags or other hacks.

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

PR Comment: https://git.openjdk.org/jdk/pull/19496#issuecomment-2213194230