RFR: 8288180: C2: VectorPhase must ensure that SafePointNode memory input is a MergeMemNode

[Tobias Hartmann]

On Thu, 8 Sep 2022 09:08:50 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> **Context:**
> The `GraphKit` seems to assume that the memory input of the map (`SafePointNode`) is always a `MergeMemNode`. It requires this so that it can easily access the memory slices.
> 
> **Analysis:**
> However, the `VectorPhase` also generates some `GraphKit` instances, for example in `PhaseVector::expand_vbox_alloc_node`. But at that point we are not in parsing, and the `SafePointNode` might have a folded memory state (not `MergeMemNode`). The assert in `GrahpKit::merged_memory` can thus be triggered.
> 
> In this particular failure case, the `SafePointNode` was constructed/initialized with memory as a memory-phi, which was the result of a previous `GraphKit::reset_memory` call, which in turn folded the memory (the `MergeMemNode` had only one input, the memory-phi). This on its own does not necessarily trigger our assert. In many cases, the new `GraphKit` first transforms the memory input and calls `GraphKit::set_all_memory`, which makes sure there is a `MergeMemNode`. But in our failure case, `GraphKit::set_all_memory` is never called before we call `GraphKit::merged_memory`.
> 
> **Side-Note:**
> The flag (`StressReflectiveCode`) was relevant because it disabled `GraphKit::get_layout_helper` from taking a constant layout helper for `T_LONG`, and instead it had to create a load (which then called `GraphKit::merged_memory`).
> 
> **Suggested Solution:**
> `VectorPhase` must ensure that the map's memory input is `MergeMemNode`. We can do this in `clone_jvms`, which is called before we instanciate the `GraphKit`.
> 
> I added a regression test, which fails without the fix, and passes with it.
> Ran regression tests, passed.

Nice analysis. Looks good to me otherwise.

src/hotspot/share/opto/vector.cpp line 168:

> 166:   Node* mem = map->memory();
> 167:   if (!mem->is_MergeMem()) {
> 168:     // Since we are not in parsing, the SafeNode does not guarantee that the memory

Suggestion:

    // Since we are not in parsing, the SafePointNode does not guarantee that the memory

src/hotspot/share/opto/vector.cpp line 170:

> 168:     // Since we are not in parsing, the SafeNode does not guarantee that the memory
> 169:     // input is necessarily a MergeMemNode. But we need to ensure that there is that
> 170:     // MereMemNode, since the GraphKit assumes the memory input of the map to be a

Suggestion:

    // MergeMemNode, since the GraphKit assumes the memory input of the map to be a

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10215