On Mon, 20 Oct 2025 05:56:03 GMT, Tobias Hartmann <[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. Thanks for the review @TobiHartmann. ------------- PR Comment: https://git.openjdk.org/valhalla/pull/1675#issuecomment-3420877016
