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

[Yagmur Eren]

On Tue, 3 Sep 2024 13:03:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>>> 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?
>
>> > 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?
> 
> No, the way the assert works is that if we fail (i.e. `failing()` is true), we do not actually want to check `s == start()` because `start()` requires `failing()` evaluating to false. So, whenever `failing()` is true, the first part of the assert makes the assertion true and we stop evaluating. But usually it is false, so we continue evaluating the second part. We also use this trick with `||`-ing conditions at other places, for example here:
> https://github.com/openjdk/jdk/blob/e0c46d589b12aa644e12e4a4c9e84e035f7cf98d/src/hotspot/share/opto/callnode.cpp#L1291
> 
> Whenever `n` is null, the first part of the assert is true and makes the entire assert true. Only if `n` is non-null, we will evaluate the second and interesting part of the assert.

Thanks a lot for the reviews and feedback @chhagedorn, @dean-long and @TobiHartmann! Can I get a sponsor?

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

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