On Mon, 13 Oct 2025 15:38:11 GMT, Damon Fenacci <[email protected]> wrote:
> ## Issue > The symptom is that an empty program is detected during loop optimization > when running `compiler/stringopts/TestStackedConcatsAppendUncommonTrap.java` > with `-XX:-TieredCompilation -XX:+StressUnstableIfTraps`. > > ## Cause > The cause's origin is string concatenation failing to bail out. More > specifically, at the end of `StringConcat::validate_control_flow`, while > validating the results, we miss processing a `Phi` node and wrongly let the > validation pass. The starting point is this: > > <img width="938" height="820" alt="image" > src="https://github.com/user-attachments/assets/b7a3ee17-33d7-4ecb-8fb0-fb962a831f04"; > /> > > While processing the `Proj` node and going through its outputs, we reach the > `CheckCastPP` and add its uses to the worklist (basically only the `Phi` > node). > > https://github.com/openjdk/valhalla/blob/708b4f92431df90c115dac840fb8194ec3aac3fe/src/hotspot/share/opto/stringopts.cpp#L1138-L1145 > > Surprisingly we don't add the `CheckCastPP` node itself. > In one of the next worklist iterations we process the `Phi` node and its uses > and none of them make the validation fail. > > Side note: `CheckCastPP` is added as additional profiling in Valhalla when > parsing `acmp` > https://github.com/openjdk/valhalla/blob/23dc125e5791238a97923108b77c6fc0b59d894f/src/hotspot/share/opto/parse2.cpp#L2098-L2102 > > On the other hand in mainline `CheckCastPP` is not added and when processing > the `Proj`, `Phi` is encountered as one of its uses and the validation fails > straight away. > > ## Fix > > Unfortunately this looks yet like another patch in the stacked optimisation's > pattern matching "denylist" approach. As mentioned by @danielogh and > @robcasloz in the JBS issue, a long-term better approach would be to limit > the concatenation to known safe cases. > > While waiting for that, a reasonable ad-hoc solution for this Valhalla case > is to add a specific `CheckCastPP` case to the validation of uses that also > adds itself to the worklist. This will include `CheckCastPP` nodes added by > Valhalla's additional profiling (in all fairness this doesn't exclude > potential other origins, but the extend and outcome in terms of concatenation > optimisation failures should be quite limited). > Note: `CastPP` has to be treated differently: it seems that not adding itself > in the worklist was done on purpose for `CastPP` to skip string null check > `Phi` nodes among other things. > > ## Testing > > Tests: tier 1-3 Thanks for the detailed analysis Damon! The fix looks good to me. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1675#pullrequestreview-3355093565
