RFR: JDK-8317299: safepoint scalarization doesn't keep track of the depth of the JVM state

[Tobias Hartmann]

On Fri, 19 Jan 2024 16:31:25 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

> # Issue
> 
> The origin of the problem is tied to the fact that, when C2 optimizes vector boxes, it performs safepoint object scalarization before late inlining.
> This can lead to situations in which scalarization adds scalarized values to the JVM state and late inlining of further methods adds further JVM state entries on top for each inlined method.
> With the example of the reported bug (_TestIntrinsicBailOut.java_) we get to a situation like this:
> 
> ...
> bc: JVMS depth=6 loc=20 stk=23 arg=23 mon=23 scalar=23 end=23 mondepth=0 sp=0 bci=1 reexecute=false method=virtual jobject jdk.incubator.vector.ByteVector.rearrangeTemplate(jobject, jobject)
> bc: JVMS depth=7 loc=23 stk=27 arg=27 mon=27 scalar=27 end=27 mondepth=0 sp=0 bci=36 reexecute=false method=virtual jobject jdk.incubator.vector.AbstractShuffle.checkIndexes()
> bc: JVMS depth=8 loc=27 stk=28 arg=28 mon=28 scalar=28 end=28 mondepth=0 sp=0 bci=1 reexecute=false method=virtual jobject jdk.incubator.vector.AbstractShuffle.reorder()
> bc: JVMS depth=9 loc=28 stk=29 arg=29 mon=29 scalar=29 end=31 mondepth=0 sp=0 bci=1 reexecute=false method=virtual jobject jdk.internal.vm.vector.VectorSupport$VectorPayload.getPayload()
> bc: JVMS depth=10 loc=31 stk=32 arg=32 mon=32 scalar=32 end=32 mondepth=0 sp=0 bci=3 reexecute=false method=static jobject jdk.internal.vm.vector.VectorSupport.maybeRebox(jobject)
> bc: JVMS depth=11 loc=32 stk=33 arg=33 mon=33 scalar=33 end=33 mondepth=0 sp=0 bci=1 reexecute=false method=virtual void jdk.internal.misc.Unsafe.loadFence()
> 
> `JVMS depth=9` shows 2 scalars but 2 further inlines added 2 more JVM states (with no scalars).
> 
> The corresponding node looks like this:
> <img width="918" alt="image" src="https://github.com/openjdk/jdk/assets/2198987/40796037-1259-41a3-8429-3ced1fdf96c9">
> 
> To keep track of its scalarized inputs, `SafePointScalarObjectNode` keeps a field `_first_index`, which is supposed to be  "relative to the last (youngest) jvms->_scloff"...
> https://github.com/openjdk/jdk/blob/c5e72450966ad50d57a8d22e9d634bfcb319aee9/src/hotspot/share/opto/callnode.hpp#L509-L511
> but if there are late inlined methods, this field is going to be relative to the JVM state at the depth before inlining happened (e.g. depth=9 in the example) and not relative to the youngest depth.
> 
> # Solution
> 
> In order to keep track of the right depth a `_depth` field is added to `SafePointScalarObjectNode`, which refers to the depth of the JVM state the `_first_index` field refers to. The method `uint first_index(JVMState*...

The fix looks good to me.

Good catch, Vladimir. I completely forgot about [JDK-8276112](https://bugs.openjdk.org/browse/JDK-8276112) but from re-reading my old analysis in https://github.com/openjdk/jdk/pull/6333, it's most likely the exact same issue.

src/hotspot/share/opto/callnode.hpp line 511:

> 509:   uint _first_index;              // First input edge relative index of a SafePoint node where
> 510:                                   // states of the scalarized object fields are collected.
> 511:   uint _depth;                    // Depth of the JVM state the first index field refers to

Suggestion:

  uint _depth;                    // Depth of the JVM state the _first_index field refers to

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17500#pullrequestreview-1848317383
PR Comment: https://git.openjdk.org/jdk/pull/17500#issuecomment-1914378760
PR Review Comment: https://git.openjdk.org/jdk/pull/17500#discussion_r1469373053