RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences [v2]

[Daniel Lundén]

On Wed, 7 May 2025 07:44:52 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> I would assume `test4` is a regression test and that the `assert` no longer triggers in any situation (otherwise we'd still have a bug)? Also, from what I can see, there is no VM flag that disables loop strip mining verification.
>> 
>> Perhaps I'm still misunderstanding you? @TobiHartmann I see you added this test back in 2021, could you help bring us some clarity? This changeset only renames the occurrence of `insert_anti_dependences` in `TestSplitIfPinnedLoadInStripMinedLoop.java` to `raise_above_anti_dependences`.
>
>> Is this test still valid?
> 
> I think it's as valid as any other regression test. With time, there's no guarantee that the test would still trigger the original issue if the fix would accidentally be reverted. But these tests still have a lot of value because they trigger state that apparently no other test triggered and often they still reproduce the issue with older JDK releases (or reveal new issues with new changes).
> 
>> but this assert appears to be removed?
> 
> It's still here, see:
> https://github.com/openjdk/jdk/blob/910d77d39e6fb9ca339272c75fa4ff7ff99bffcf/src/hotspot/share/opto/gcm.cpp#L889-L890
> 
>> does the test on top of which the comment is placed (test4 right?) ever run with loop strip mining verification disabled and if it does, does the assert get triggered?
> 
> It's not possible to disable loop strip mining but IIRC I disabled it manually to see what other issues we hit without verification, for example in release builds (see also my comments in https://github.com/openjdk/jdk/pull/2315). So this test basically covers a different failure mode.
> 
> I think Daniel's change is good as it is.

Thanks @TobiHartmann. Note that this changeset does remove the assert, and replaces it with another (stronger, in theory) assert.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077109945