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

[Theo Weidmann]

On Wed, 8 Jan 2025 13:48:29 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Theo Weidmann has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix test name
>
> src/hotspot/share/opto/stringopts.cpp line 613:
> 
>> 611:         } else if (use != nullptr &&
>> 612:                    check_append_candidate(use, sc, m, string_sig, int_sig, char_sig) == CheckAppendResult::GiveUp) {
>> 613:           return nullptr;
> 
> What happens here in the two other cases `GoodAppend` and `NotAppend`?

We don't really care here if it was an append or not. The only important thing is to exit this algorithm if the processing of the append candidate detected a reason to definitely give up.

> src/hotspot/share/opto/stringopts.cpp line 643:
> 
>> 641: 
>> 642:       if (result == CheckAppendResult::GiveUp) {
>> 643:         break;
> 
> Can you put a comment here where this is supposed to jump, and why?

I was already considering to replace all the `break` with `return nullptr` here before, because all of them just exit from the entire algorithm because we detected something that completely prevents this optimization. But then I thought I will rather stick with the existing pattern and not change code that does not really need to be changed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907226706
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907229909