Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
On Tue, 28 Sep 2021 11:49:29 GMT, Claes Redestad wrote: >> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to >> work also for ASCII encoding, which makes for example the `UTF_8$Encoder` >> perform on par with (or outperform) similarly getting charset encoded bytes >> from a String. The former took a small performance hit in JDK 9, and the >> latter improved greatly in the same release. >> >> Extending the `EncodeIsoArray` intrinsics on other platforms should be >> possible, but I'm unfamiliar with the macro assembler in general and unlike >> the x86 intrinsic they don't use a simple vectorized mask to implement the >> latin-1 check. For example aarch64 seem to filter out the low bytes and then >> check if there's any bits set in the high bytes. Clever, but very different >> to the 0xFF80 2-byte mask that an ASCII test wants. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add fail-safe predicate to all encode_iso_array to only match non-ASCII > EncodeISOArrayNodes The latest version looks good to me. - Marked as reviewed by thartmann (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
Yes, this was spotted and fixed already. Annoyingly the test I added didn't detect this so GHA was green, but it failed some tier2 tests on aarch64. I added extra safeguards by predicating matching the encode_iso_array instructions on the node being !ascii, which will cause C2 to report an error rather than silently using the iso variant for ascii-only nodes. Hämta Outlook för Android<https://aka.ms/AAb9ysg> From: core-libs-dev on behalf of Volker Simonis Sent: Tuesday, September 28, 2021 1:53:22 PM To: core-libs-dev@openjdk.java.net ; hotspot-compiler-...@openjdk.java.net ; nio-...@openjdk.java.net Subject: Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5] On Tue, 28 Sep 2021 10:01:43 GMT, Claes Redestad wrote: >> Not too much work. I recently introduced platform-specific `matcher_*.hpp` >> files, so since then adding a boolean constant is easy (no need to muck with >> the .ad files). > > Does the changes in 9800a99 resolve your concerns? In principle yes, but shouldn't the condition read: if (!Matcher::match_rule_supported(Op_EncodeISOArray) || !Matcher::supports_encode_ascii_array) return false; I.e. the intrinisc is supported if both conditions are true and not supported if either one of them is false? - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
On Tue, 28 Sep 2021 10:01:43 GMT, Claes Redestad wrote: >> Not too much work. I recently introduced platform-specific `matcher_*.hpp` >> files, so since then adding a boolean constant is easy (no need to muck with >> the .ad files). > > Does the changes in 9800a99 resolve your concerns? In principle yes, but shouldn't the condition read: if (!Matcher::match_rule_supported(Op_EncodeISOArray) || !Matcher::supports_encode_ascii_array) return false; I.e. the intrinisc is supported if both conditions are true and not supported if either one of them is false? - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add fail-safe predicate to all encode_iso_array to only match non-ASCII EncodeISOArrayNodes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/47633cba..b4a5d105 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=03-04 Stats: 3 lines in 3 files changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
