RFR: 8261730: C2 compilation fails with assert(store->find_edge(load) != -1) failed: missing precedence edge

[Tobias Hartmann]

On Mon, 1 Mar 2021 09:53:02 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> This change relaxes an assertion in `PhaseCFG::verify()` to accept the case where a store is used to implement an implicit null check and a load is placed in the null block.
> 
> The implicit null check transformation `PhaseCFG::implicit_null_check()` tries to reuse globally scheduled memory operations to perform null checks, hoisting the memory operations into null-check blocks if necessary. When hoisting stores, `implicit_null_check()` preserves anti-dependences between the hoisted store and loads in the corresponding null-check and not-null blocks, but disregards possible anti-dependences with loads in the null block. This is safe, as the null block is only executed when the hoisted store has "failed" (attempted to write to the null address). This change makes `PhaseCFG::verify()` aware of this fact to avoid falsely reporting an anti-dependence violation (missing precedence edge from the load in the null block to the store hoisted to the null-check block).
> 
> **Example.** After global scheduling ([before-implicit-null-check.pdf](https://github.com/openjdk/jdk/files/6060581/before-implicit-null-check.pdf)), `implicit_null_check()` detects a store `(17 storeImmIO)` in B6 that could potentially perform the null check done by `(5 testN_reg)` in B4. After checking that neither B4 or B6 contain anti-dependent loads, the transformation replaces `(5 testN_reg)` with `(17 storeImmIO)` by hoisting the latter into B4 and adding an additional `(46 NullCheck)` pseudo-operation ([after-implicit-null-check.pdf](https://github.com/openjdk/jdk/files/6060614/after-implicit-null-check.pdf)). Finally, after local scheduling, `PhaseCFG::verify()` falsely reports that `(17 storeImmIO)` can overwrite the memory value read by the load operation `(34 subI_rReg_mem)` in the null block B5. This overwrite cannot happen, because executing B5 implies `(17 storeImmIO)`'s failure to store any value.
> 
> Tested on hs-tier1-5 (windows-x64, linux-x64, linux-aarch64, and macosx-x64).

Nice analysis. Your fix looks good to me (added a minor comment).

src/hotspot/share/opto/gcm.cpp line 789:

> 787:         }
> 788: #endif
> 789:         assert((store_null_check != NULL && LCA == null_block) ||

I think you could avoid the `store_null_check != NULL` check in the assert because `LCA` is always non-null.

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

Marked as reviewed by thartmann (Reviewer).

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