RFR: 8333893: Optimization for StringBuilder append boolean & null [v4]

[Emanuel Peter]

On Wed, 12 Jun 2024 14:24:00 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> _putCharStringU will perform MergeStore but with bounds checking. Can the C2 optimizer perform MergeStore without bounds checking in this case?
>> 
>> StringUTF16.putChar does not have a bounds check, so there should be no bounds check after MergeStore?
>> 
>> 
>> class AbstractStringBuilder {
>>     private AbstractStringBuilder appendNull() {
>>         // ...
>>         StringUTF16.putCharsAt(val, count, 'n', 'u', 'l', 'l');
>>         // ...
>>     }
>> }
>> 
>> class StringUTF16 {
>>   	public static void putCharsAt(byte[] value, int i, char c1, char c2, char c3, char c4) {
>>         putChar(value, i    , c1);
>>         putChar(value, i + 1, c2);
>>         putChar(value, i + 2, c3);
>>         putChar(value, i + 3, c4);
>>     }
>> 
>>  	@IntrinsicCandidate
>>     // intrinsic performs no bounds checks
>>     static void putChar(byte[] val, int index, int c) {
>>         assert index >= 0 && index < length(val) : "Trusted caller missed bounds check";
>>         index <<= 1;
>>         val[index++] = (byte)(c >> HI_BYTE_SHIFT);
>>         val[index]   = (byte)(c >> LO_BYTE_SHIFT);
>>     }
>> }
>
> @wenshao If you look at the tests in https://github.com/openjdk/jdk/pull/16245, you can see examples like this:
> https://github.com/eme64/jdk/blob/93bf2ddc9b7f584724034aec6a4f8b9fe1b2dfda/test/hotspot/jtreg/compiler/c2/TestMergeStores.java#L501-L514
> So yes, we can merge them.

> @eme64 It seems like MergeStore didn't happen, is there something I did wrong?

Yes ;)

@wenshao The issue is that the pattern matching is quite **limited**.
`putCharsAt(byte[] value, int i, char c1, char c2, char c3, char c4)`
This has 4 variables. This is not allowed for the optimization. You would have to pass in a `long` and split it via shifts.
A possible exception: if `putCharsAt` is inlined and outside it is a splitting of a `long` value with shift, then ... maybe ... this would be optimized with `MergeStores`.

There are only 2 **accepted** patterns:
- Multiple stores of constants -> constant is merged, which allows stores to be merged.
- Multiple stores which all store a "section" of a larger value:

You can see **examples** I listed at the beginning of the PR description in https://github.com/openjdk/jdk/pull/16245.

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

PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2163261884