RFR: 8357105: C2: compilation fails with "assert(false) failed: empty program detected during loop optimization"

[Roberto Castañeda Lozano]

On Thu, 22 May 2025 15:19:08 GMT, Daniel Skantz <dskantz at openjdk.org> wrote:

> This pull request contains a fix for JDK-8357105.
> 
> The problem is performing stacked string concatenation optimization between a pair of StringBuilder.append().toString()-links SB1 and SB2, where the parameter of an append call in SB2 has a complex dependency on the result of SB1, which in turn is replaced by top() during stringopts -- similar to JDK-8271341, which had a diamond if-structure using the result of SB1, while in this case the use is an unstable If. In the attached regression test, a live part of the graph gets optimized away during later phases and ultimately the whole graph vanishes.
> 
> The proposed solution is to simply exclude this specific case. This bug has existed for a long time and stacked concats is a niche optimization.
> 
> Testing:
> Tier1-4.
> 
> Extra testing:
> Ran Tier1-4 with an instrumented build and observed that we do not disable stacked concatenation in any previously known case after the fix.

The overall analysis and fix look good to me, I just have some minor style, test, and code comment suggestions.

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

> 989: 
> 990:       // A test which leads to an uncommon trap which could be safe.
> 991:       // If so, this trap will later be converted into a trap that restarts

I suggest to make the safety condition clearer in the comment, something like this:
Suggestion:

      // A test which leads to an uncommon trap. It is safe to convert the trap
      // into a trap that restarts at the beginning as long as its test does not
      // depend on intermediate results of the candidate chain.

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

> 994:         CallStaticJavaNode* call = otherproj->unique_out()->isa_CallStaticJava();
> 995:         if (call != nullptr && call->_name != nullptr && strcmp(call->_name, "uncommon_trap") == 0) {
> 996:           // first check for dependency on a toString that is going away during stacked concats.

Suggestion:

          // First check for dependency on a toString that is going away during stacked concats.

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

> 996:           // first check for dependency on a toString that is going away during stacked concats.
> 997:           if (_multiple && ((v1->is_Proj() && is_SB_toString(v1->in(0)) && ctrl_path.member(v1->in(0)))
> 998:                             || (v2->is_Proj() && is_SB_toString(v2->in(0)) && ctrl_path.member(v2->in(0))))) {

For consistency with the surrounding code, and to make the symmetry between the two cases more obvious:
Suggestion:

          if (_multiple &&
              ((v1->is_Proj() && is_SB_toString(v1->in(0)) && ctrl_path.member(v1->in(0))) ||
               (v2->is_Proj() && is_SB_toString(v2->in(0)) && ctrl_path.member(v2->in(0))))) {

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

> 28:  *          of the first StringBuilder chain is wired into an uncommon trap
> 29:  *          located in the second one.
> 30:  * @run main/othervm compiler.stringopts.TestStackedConcatsAppendUncommonTrap

Please add a second run that is constrained via JVM flags to be more stable and easier to analyze, using `-Xbatch`, `-XX:CompileOnly=...`, and perhaps `-XX:-TieredCompilation`. Using `-Xbatch` also allows you to reduce the number of warm-up iterations, many tests use `10_000`.

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25395#pullrequestreview-2864732620
PR Review Comment: https://git.openjdk.org/jdk/pull/25395#discussion_r2104781146
PR Review Comment: https://git.openjdk.org/jdk/pull/25395#discussion_r2104782240
PR Review Comment: https://git.openjdk.org/jdk/pull/25395#discussion_r2104785775
PR Review Comment: https://git.openjdk.org/jdk/pull/25395#discussion_r2104792453