RFR: 8341696: C2: Non-fluid StringBuilder pattern bails out in OptoStringConcat [v4]

[Emanuel Peter]

On Thu, 12 Dec 2024 09:55:24 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> Extends stringopts to also recognize non-fluid uses of StringBuilder and optimize them the same way.
>> 
>> For example, this basic case was not optimized before and is optimized with this PR:
>> 
>> 
>> StringBuilder sb = new StringBuilder();
>> sb.append("a");
>> sb.append(a);
>> return sb.toString();
>
> Theo Weidmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix test name

Looks promising. Though I did not work with string-opts before, so a little hard for me to give a proper review. If you want/need me to review, a few more annotations with github comments would help. Otherwise I'll just leave it at the drive-by comments ;)

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

> 412: }
> 413: 
> 414: PhaseStringOpts::CheckAppendResult PhaseStringOpts::check_append_candidate(CallStaticJavaNode* cnode,

Using two verbs in succession is a little confusing. At least write `check_and_append_candidate`. Or maybe `append_candidate_if_<something>`.

test/hotspot/jtreg/compiler/stringopts/TestFluidAndNonFluid.java line 27:

> 25:  * @test
> 26:  * @bug 8341696
> 27:  * @requires vm.compiler2.enabled

Not sure if I asked about this already: do we need this C2 restriction? The IR framework only checks IR  rules for C2, but the test could still do value verification for other settings where C2 is not available.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22537#pullrequestreview-2537099237
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907161513
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907155428