RFR: 8343148: C2: Refactor uses of "PhaseValue::*con*() + PhaseIdealLoop::set_ctrl()" into separate method [v12]

[theoweidmannoracle]

On Thu, 7 Nov 2024 17:48:27 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>>> Do we have other places (not new constant node) where we set Root as control? May be we can add `set_root_as_ctrl(n)` method in `loop node.hpp` in such case.
>> 
>> There's only three locations where control is set to the root in the loop files now (not counting the ones in the new methods I added). The main reason for this patch is bugs caused by people forgetting to set control for constants (e.g. https://bugs.openjdk.org/browse/JDK-8343137), which is now prevented if the new helper methods are used.
>> 
>> Do you think there would be any benefit from introducing `set_root_as_ctrl(n)` given there's only about three places where this pattern occurs now?
>
>> > Do we have other places (not new constant node) where we set Root as control? May be we can add `set_root_as_ctrl(n)` method in `loop node.hpp` in such case.
>> 
>> There's only three locations where control is set to the root in the loop files now (not counting the ones in the new methods I added). The main reason for this patch is bugs caused by people forgetting to set control for constants (e.g. https://bugs.openjdk.org/browse/JDK-8343137), which is now prevented if the new helper methods are used.
>> 
>> Do you think there would be any benefit from introducing `set_root_as_ctrl(n)` given there's only about three places where this pattern occurs now?
> 
> My suggesting is about additional cleaning code. I think 3 + 5 places are enough to justify to have a new function in header file. Also `set_root_as_ctrl(n)` could be copy of `set_ctrl(n, ctrl)` without 2 asserts which checks `ctrl`. It will be faster.

@vnkozlov Could you take another look at the latest changes?

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

PR Comment: https://git.openjdk.org/jdk/pull/21836#issuecomment-2504177215