RFR: 8330159: [C2] Remove or clarify Compile::init_start [v5]

[Yagmur Eren]

On Mon, 2 Sep 2024 11:12:33 GMT, Yagmur Eren <duke at openjdk.org> wrote:

>> Yagmur Eren has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   adding NOT_DEBUG_RETURN instead of DEBUG_ONLY for Compile::verify_start
>
> Thanks a lot for the review and suggestions @dean-long and @chhagedorn! I believe I can integrate now if it looks good!

> Hi @nelanbu, I don't think this is correct. In `Compile::start()`, we have the following code:
> 
> https://github.com/openjdk/jdk/blob/b8e8e965e541881605f9dbcd4d9871d4952b9232/src/hotspot/share/opto/compile.cpp#L1121-L1131
> 
> It asserts that `failing()` is false. Therefore, `init_start()` bails out before checking the assert with `start()` which you now no longer do with your refactoring.
> 
> What you could do instead:
> 
> * Simplify the code in `init_start()` to and add an assertion message:
> 
> ```
>   assert(failing() || s == start(), "should be StartNode");
> ```
> 
> * Change `init_start_node()` into a more meaningful name like `verify_start()`, as we are not actually initializing anything but rather sanity checking the start node.
> * Guard the method with `DEBUG_ONLY/ifdef ASSERT` since it's only calling an assert in debug VM and nothing in product VM.


Is `failing()` true if it fails as the name suggests? If so, then I guess it should be `!failing()` within `assert`, right?

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

PR Comment: https://git.openjdk.org/jdk/pull/20715#issuecomment-2326427885