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

[Theo Weidmann]

On Wed, 8 Jan 2025 13:58:02 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> 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.

If we had exceptions, the method could just return true and false, and throw to abort the optimization...

>> 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.

Of course the way this entire loop works is a little confusing, but I didn't want to really refactor/rewrite all of this as it seems quite delicate and not trivial to test.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907233513
PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907232574