Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Hohensee, Paul
My apologies. I relied on the other reviewers. I'll do an independent review in the future. Thanks, Paul On 10/14/20, 11:02 AM, "core-libs-dev on behalf of Roger Riggs" wrote: On Wed, 14 Oct 2020 15:01:42 GMT, Roger Riggs wrote: >> Due to the requirement for multiple reviewers, I

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Daniel D . Daugherty
On Wed, 14 Oct 2020 17:59:53 GMT, Roger Riggs wrote: >> This integration without testing with a current merge from the master and >> has caused two build failures. >> >> JDK-8254761: Wrong intrinsic annotation used for StringLatin1.indexOfChar >> >> JDK-8254775: Microbenchmark

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Roger Riggs
On Wed, 14 Oct 2020 15:01:42 GMT, Roger Riggs wrote: >> Due to the requirement for multiple reviewers, I had been waiting to add my >> review of the Core-Libs files until the >> HotSpot reviewers had approved! I see only one reviewer credited in the >> commit. > > This integration without

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Roger Riggs
On Wed, 14 Oct 2020 13:18:19 GMT, Roger Riggs wrote: >> Marked as reviewed by neliasso (Reviewer). > > Due to the requirement for multiple reviewers, I had been waiting to add my > review of the Core-Libs files until the > HotSpot reviewers had approved! I see only one reviewer credited in the

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Roger Riggs
On Wed, 14 Oct 2020 12:03:42 GMT, Nils Eliasson wrote: >> Jason Tatton has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added missing copyright notices > > Marked as reviewed by neliasso (Reviewer). Due to the requirement for multiple

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v6]

2020-10-14 Thread Nils Eliasson
On Mon, 12 Oct 2020 11:17:25 GMT, Jason Tatton wrote: >> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 >> byte encoded Strings). It is provided for >> x86 and ARM64. The implementation is greatly inspired by the indexOf(char) >> intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v3]

2020-09-22 Thread Volker Simonis
On Mon, 21 Sep 2020 12:45:55 GMT, Jason Tatton wrote: >> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 >> byte encoded Strings). It is provided for >> x86 and ARM64. The implementation is greatly inspired by the indexOf(char) >> intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v2]

2020-09-22 Thread Volker Simonis
On Fri, 18 Sep 2020 11:56:09 GMT, Jason Tatton wrote: >> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 >> byte encoded Strings). It is provided for >> x86 and ARM64. The implementation is greatly inspired by the indexOf(char) >> intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-22 Thread Andrew Dinn
On Tue, 22 Sep 2020 02:31:14 GMT, Vladimir Kozlov wrote: >>> >Can you explain where this restricted effect is documented? >> >>> Certainly! I’ve found that determining the capability of the CPU and >>> whether to enable AVX2 support if the chip >>> supports it is mostly controlled in:

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-21 Thread mknjc
On Fri, 18 Sep 2020 23:11:46 GMT, Jason Tatton wrote: >> "the JVM has knowledge of the AVX capability of the chip it’s running on and >> disables the AVX2 code path for chips >> which suffer from the performance degradation which has been outlined in >> this discussion" >> Does it? The white

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-21 Thread Vladimir Kozlov
On Mon, 21 Sep 2020 09:20:56 GMT, Andrew Dinn wrote: >>> Can you explain where this restricted effect is documented? >> >> Certainly! I’ve found that determining the capability of the CPU and whether >> to enable AVX2 support if the chip >> supports it is mostly controlled in:

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v3]

2020-09-21 Thread Jason Tatton
> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 > byte encoded Strings). It is provided for > x86 and ARM64. The implementation is greatly inspired by the indexOf(char) > intrinsic for StringUTF16. To incorporate it > I had to make a small change to

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v2]

2020-09-21 Thread Volker Simonis
On Fri, 18 Sep 2020 11:56:09 GMT, Jason Tatton wrote: >> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 >> byte encoded Strings). It is provided for >> x86 and ARM64. The implementation is greatly inspired by the indexOf(char) >> intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-21 Thread Andrew Dinn
On Fri, 18 Sep 2020 23:11:46 GMT, Jason Tatton wrote: > >Can you explain where this restricted effect is documented? > Certainly! I’ve found that determining the capability of the CPU and whether > to enable AVX2 support if the chip > supports it is mostly controlled in:

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Jason Tatton
On Fri, 18 Sep 2020 16:31:23 GMT, Andrew Dinn wrote: > Can you explain where this restricted effect is documented? Certainly! I’ve found that determining the capability of the CPU and whether to enable AVX2 support if the chip supports it is mostly controlled in: [vm_version_x86.cpp](

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Andrew Dinn
On Fri, 18 Sep 2020 15:55:52 GMT, Jason Tatton wrote: >> Hi Andrew, >> >> Thanks for coming back to me. Looking on the github >> [PR](https://github.com/openjdk/jdk/pull/71) nobody is tagged as a >> reviewer for this (perhaps this is a feature which is not being used). >>> That's >>> because

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Jason Tatton
On Fri, 18 Sep 2020 11:04:34 GMT, Jason Tatton wrote: >> Hi everyone, >> >> This patch is just missing a couple of reviewers... Please can someone step >> forward? >> >> I think it's a fairly straightforward change. >> >> -Jason > > Hi Andrew, > > Thanks for coming back to me. Looking on

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Andrew Dinn
On 18/09/2020 12:06, Jason Tatton wrote: > There are two separate things here: > 1). Justification for the change itself: > -The objective and justification for this patch is to bring the performance > of StringLatin1 indexOf(char) in > line with StringUTF16 indexOf(char) for x86 and ARM64.

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char) [v2]

2020-09-18 Thread Jason Tatton
> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 > byte encoded Strings). It is provided for > x86 and ARM64. The implementation is greatly inspired by the indexOf(char) > intrinsic for StringUTF16. To incorporate it > I had to make a small change to

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Jason Tatton
On Thu, 17 Sep 2020 18:20:08 GMT, Jason Tatton wrote: >> Hi Andrew, >> >> The current indexOf char intrinsics for StringUTF16 and the new one here for >> StringLatin1 both use the AVX2 – i.e. 256 >> bit instructions, these are also affected by the frequency scaling which >> affects the

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-18 Thread Andrew Dinn
On 17/09/2020 19:22, Jason Tatton wrote: > This patch is just missing a couple of reviewers... Please can someone step > forward? > > I think it's a fairly straightforward change. I believe you got a review from Andrew Haley -- it was quoted in your follow-up from which I selected the above

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-17 Thread Jason Tatton
On Fri, 11 Sep 2020 23:04:01 GMT, Jason Tatton wrote: >> This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 >> byte encoded Strings). It is provided for >> x86 and ARM64. The implementation is greatly inspired by the indexOf(char) >> intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-14 Thread Andrew Haley
On 12/09/2020 00:06, Jason Tatton wrote: > The current indexOf char intrinsics for StringUTF16 and the new one > here for StringLatin1 both use the AVX2 – i.e. 256 bit instructions, > these are also affected by the frequency scaling which affects the > AVX-512 instructions you pointed out. Of

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-11 Thread Jason Tatton
On Tue, 8 Sep 2020 11:59:36 GMT, Jason Tatton wrote: > This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 > byte encoded Strings). It is provided for > x86 and ARM64. The implementation is greatly inspired by the indexOf(char) > intrinsic for StringUTF16. To

Re: RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-11 Thread Andrew Haley
On 11/09/2020 11:23, Jason Tatton wrote: > For the x86 implementation there may be two further improvements we > can make in order to improve performance of both the StringUTF16 and > StringLatin1 indexOf(char) intrinsics: > > 1. Make use of AVX-512 instructions. Is this really a good idea?

RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

2020-09-11 Thread Jason Tatton
This is an implementation of the indexOf(char) intrinsic for StringLatin1 (1 byte encoded Strings). It is provided for x86 and ARM64. The implementation is greatly inspired by the indexOf(char) intrinsic for StringUTF16. To incorporate it I had to make a small change to StringLatin1.java