RFR: 8356246: C2: Compilation fails with "assert(bol->is_Bool()) failed: unexpected if shape" in StringConcat::eliminate_unneeded_control

[Roberto Castañeda Lozano]

On Tue, 27 May 2025 07:56:59 GMT, Daniel Skantz <dskantz at openjdk.org> wrote:

> This pull request contains a fix for JDK-8356246.
> 
> During stacked concatenations, a pair of `StringBuilder.append().toString()` links SB and SB2 could have a diamond if structure `(Region -> <IfTrue, IfFalse> -> If)` created by String.valueOf that depends on the return value of SB1, which is going away (replaced by top() in `eliminate_call` in stringopts).
> 
> JDK-8271341 added folding of the region of the diamond-if to stringopts to avoid the case where a live part of the graph becomes unreachable as this top() propagates through the graph too quickly.
> 
> JDK-8291775 was a follow-up fix and instead used a constant test as input to the diamond If, as a case was discovered where the If was processed before the Region leading to a broken graph.
> 
> The code in JDK-8271341 assumes that the input to the If is a boolean, not a constant. If two diamond if-region structures in the same StringBuilder candidate share the same test, the second iteration in `eliminate_unneeded_control` will fail with an unexpected input. The proposed solution is to skip over the second iteration as the test has already been replaced by a constant -- both structures will be simplified independently during IGVN.
> 
> Testing:
> T1-4.

Thanks for finding, reporting, and fixing this issue Daniel! The analysis and fix look good to me, I only have a few minor comments and suggestions.

src/hotspot/share/opto/stringopts.cpp line 263:

> 261:         // was replaced by a constant zero in a previous call to this method.
> 262:         // Do nothing as the transformation in the previous call ensures both are folded away.
> 263:         assert(bol == _stringopts->gvn()->intcon(0), "set below.");

Could you make the assertion failure message more informative? E.g. something like:

Suggestion:

        assert(bol == _stringopts->gvn()->intcon(0), "shared condition should have been set to false");

test/hotspot/jtreg/compiler/stringopts/TestStackedConcatsSharedTest.java line 30:

> 28:  *          is used as a shared test by two diamond Ifs in the second StringBuilder.
> 29:  * @run main/othervm compiler.stringopts.TestStackedConcatsSharedTest
> 30:  * @run main/othervm -Xbatch -XX:-TieredCompilation -Xcomp

`-Xcomp` already implies `-Xbatch`:

Suggestion:

 * @run main/othervm -XX:-TieredCompilation -Xcomp

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25461#pullrequestreview-2870772010
PR Review Comment: https://git.openjdk.org/jdk/pull/25461#discussion_r2109063255
PR Review Comment: https://git.openjdk.org/jdk/pull/25461#discussion_r2108990869