On Thu, 20 Jun 2024 10:09:46 GMT, Claes Redestad <redes...@openjdk.org> wrote:
>> @wenshao I'm glad we figured it out a bit, and I hope you learned a thing or >> two :) >> >>> It would be even better if Unsafe.putChar could be used for MergeStore in >>> the future. >> >> The `_putCharStringU` intrinsic uses >> `LibraryCallKit::inline_string_char_access`. It seems to generate IR nodes, >> I speculate it could be `StoreC`. We'd have to do more digging to see why >> that pattern is not accepted by `MergeStore`. >> >> @wenshao I'm not sure if everything is right with the bounds checking, but I >> leave this to the library folks to review. What you will definately need is >> benchmarking not just on `M1`, but also `x86-64` and other `aarch64` >> architectures. > > I'm not opposed to accepting this patch as-is, but I think we should do so > with an eye towards reverting if we figure out a way to improve the `putChar` > intrinsic so that it doesn't block merge store optimization. What do you > think @eme64? > > As the need for the changes in > [b5ad8e7](https://github.com/openjdk/jdk/pull/19626/commits/b5ad8e70928c547d134d0e4a532441cad9a7e4a2) > showed added code complexity (like here) can be detrimental to performance, > and if the `putChar` can be improved we might see benefits in more places. @cl4es @wenshao I think we should probably work on `putChar`, or at least figure out what blocks `MergeStore` for `putChar`. Can someone produce a simple stand-alone `.java` file that uses `putChar`, so that I can investigate why `MergeStore` does not work for it? Otherwise, I agree with @cl4es : the code here may be ok for now, but we would have to revert it again later, should `MergeStore` eventually do the trick. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2180388366
