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

[Tobias Hartmann]

On Tue, 6 May 2025 18:02:55 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

>> Ok, maybe I was not clear enough.
>> 
>> The comment says:
>> 
>>>     // Triggers an assert in PhaseCFG::raise_above_anti_dependences if loop strip mining verification is disabled:
>> 
>> My question is, 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? If `test4` does not do this, seems to me it would be nice to have an additional test that verifies just that rather than accept/assume the comment as valid without a test that actually verifies this. Thoughts?
>
> 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.

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

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