RFR: 8258894: C2: Forbid GCM to move stores into loops

[Tobias Hartmann]

On Tue, 19 Jan 2021 08:32:42 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> Prevent GCM from placing memory-writing nodes (such as stores) into loops deeper than their home loop (determined by their control input). Such placements are invalid, as they cause memory definitions to interfere, and risk causing miscompilations. This change complements [JDK-8255763](https://bugs.openjdk.java.net/browse/JDK-8255763), which only addresses invalid placements in irreducible CFGs.
> 
> Add control input to stores in generated stubs to ensure that all memory-writing nodes have control inputs from which their home block can be derived.
> 
> Add a battery of simplified fuzzer test cases where, before this change, GCM moves stores into deeper loops.
> 
> Tested functionality on:
> - hs-tier1-5 (windows-x64, linux-x64, linux-aarch64, and macosx-x64; release and debug mode)
> - hs-tier6-9 (linux-x64, debug mode)
> 
> Tested performance on a set of standard benchmark suites (DaCapo, Renaissance, SPECjbb2015, SPECjbb2005, SPECjvm2008, ...) and on windows-x64, linux-x64, and macosx-x64. No regression was observed, which is expected since this change only affects the placement of memory-writing nodes (which tends to be quite constrained already), and only forbids placements which would be typically detrimental for performance anyway (moving such nodes into deeper loops).

Looks good to me. Just to clarify, with JDK-8255763 in place, incorrect placement in the reducible CFG case is always prevented by the frequency estimation heuristic, right? I.e. without this patch, your newly added tests wouldn't fail? Would it make sense to run the test with `StressGCM` to trigger actual failures?

src/hotspot/share/opto/block.cpp line 1211:

> 1209: }
> 1210: 
> 1211: void PhaseCFG::verify_memory_writer_placement(const Block* b, const Node* n) const {

You could wrap the entire method in `#ifdef ASSERT` and use `NOT_DEBUG_RETURN` in the .hpp (similar to  `verify_strip_mined`).

src/hotspot/share/opto/block.cpp line 1241:

> 1239:       }
> 1240:       if (n->is_memory_writer()) {
> 1241:         verify_memory_writer_placement(block, n);

Other calls to this method are guarded by `#ifdef ASSERT`. Would it make sense to move the `n->is_memory_writer()` into the method, always call it and define it as `NOT_DEBUG_RETURN`?

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

Marked as reviewed by thartmann (Reviewer).

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