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

[Brian J. Stafford]

On Thu, 19 May 2022 11:54:59 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> Brian J. Stafford has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Removed whitespace
>>  - Added braces for if statements
>
> 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.

Removed.

> 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`._

Thank you, I've updated the comments as you've suggested.

> 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.

Moved. I added braces around the `continue` to match the expected coding convention.

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

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