RFR: 8263075: C2: simplify anti-dependence check in PhaseCFG::implicit_null_check() [v2]

[Roberto Castañeda Lozano]

On Tue, 17 May 2022 20:19:14 GMT, Brian J. Stafford <duke at openjdk.java.net> wrote:

>> The reporter for this issue (https://bugs.openjdk.java.net/browse/JDK-8263075) indicated that there's an assumption that we can rely on that the while loop in question will run exactly one time. Based on this, I've done the following:
>> 
>> - Asserted the condition that makes sure the code runs at least once
>> - Asserted the condition that makes sure the code runs only once
>> - Removed the `while` loop
>> - Changed a couple of `break` statements into `continue` statements. They no longer need to break out of the `while` loop, now that it's gone. However, they were early exits from the `while` loop that ended up resulting in `continue` statements for the larger enclosing loop. Thus we can just call `continue` directly.
>> - Removed the local variable `b`, as we no longer need to traverse the node hierarchy. We can use `mb` directly.
>> 
>> Passes jdk, langtools, and hotspot Tier 1 tests on Linux (x64 and ARM64) and macOS (x64 and ARM64). Most Tier 1 tests pass on Windows (x64 and ARM64), but there are a handful of failures unrelated to this change.
>
> Brian J. Stafford has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Removed whitespace
>  - Added braces for if statements

Thanks for working on this cleanup, @brianjstafford! The loop removal looks good to me, and the additional tests passed, I just have a few comments about the assertions and their documentation.

src/hotspot/share/opto/lcm.cpp line 330:

> 328:     // Give up hoisting if we have to move the store past any load.
> 329:     if (was_store) {
> 330:        // Start searching here for a local load

This comment is obsolete and can be removed, as it implicitly refers to a loop that no longer exists.

src/hotspot/share/opto/lcm.cpp line 333:

> 331:        // mach use (faulting) trying to hoist
> 332:        // n might be blocker to hoisting
> 333:        // This assert ensures that the following code should be run

This comment and the similar one for the second assertion are also obsolete for the same reason. Instead, you could add a comment explaining why we expect `get_block_for_node(mb->pred(1)) == block`. I suggest something along these lines: _`mach` is a store, hence `block` is the immediate dominator of `mb`. Due to the null-check shape of `block` (where its successors cannot re-join), `block` must be the direct predecessor of `mb`._

src/hotspot/share/opto/lcm.cpp line 352:

> 350: 
> 351:        // This assert ensures that the above code should only be run once
> 352:        assert(get_block_for_node(mb->pred(1)) == block, "Unexpected predecessor block");

The single-predecessor test and the assertion `get_block_for_node(mb->pred(1)) == block` can be moved above the for-loop to make the assertion `mb != block` redundant.

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

Changes requested by rcastanedalo (Committer).

PR: https://git.openjdk.java.net/jdk/pull/8684