Looks good to me too.
Please, push into jdk9/hs-comp so we get testing of intrinsic in Hotspot
nightly testing.
Thanks,
Vladimir
On 6/17/15 3:31 PM, Anthony Scarpino wrote:
On 06/15/2015 05:20 PM, John Rose wrote:
Thanks for taking this on.
It looks good, except for one thing. The intrinsi
Thumbs up. Thanks! — John
On Jun 17, 2015, at 3:31 PM, Anthony Scarpino
wrote:
>
> On 06/15/2015 05:20 PM, John Rose wrote:
>> Thanks for taking this on.
>>
>> It looks good, except for one thing. The intrinsic does not need to
>> be an instance method, and doing so creates an undesirable
On 06/15/2015 05:20 PM, John Rose wrote:
Thanks for taking this on.
It looks good, except for one thing. The intrinsic does not need to
be an instance method, and doing so creates an undesirable coupling
between the JVM and JDK. Specifically, the JDK should not need to
know about subkeyH and s
On Jun 17, 2015, at 10:40 AM, Anthony Scarpino
wrote:
>
> On 06/15/2015 05:20 PM, John Rose wrote:
>> Thanks for taking this on.
>>
>> It looks good, except for one thing. The intrinsic does not need to be
>> an instance method, and doing so creates an undesirable coupling between
>> the JVM an
On 06/15/2015 05:20 PM, John Rose wrote:
Thanks for taking this on.
It looks good, except for one thing. The intrinsic does not need to be
an instance method, and doing so creates an undesirable coupling between
the JVM and JDK. Specifically, the JDK should not need to know about
subkeyH and sta
Thanks for taking this on.
It looks good, except for one thing. The intrinsic does not need to be an
instance method, and doing so creates an undesirable coupling between the JVM
and JDK. Specifically, the JDK should not need to know about subkeyH and state
fields. The Java code should pass
Hi other reviewers,
I'm hoping someone else can review this.
Tony
On 06/12/2015 03:16 PM, Vladimir Kozlov wrote:
This implementation looks good to me. You need second review since
changes are big.
Thanks,
Vladimir
On 6/11/15 11:01 AM, Anthony Scarpino wrote:
Now that JEP 246 is targeted, I
This implementation looks good to me. You need second review since
changes are big.
Thanks,
Vladimir
On 6/11/15 11:01 AM, Anthony Scarpino wrote:
Now that JEP 246 is targeted, I can proceed a with the GHASH changes I
had reviewed back in February.
There are two changes from the previous revie
Now that JEP 246 is targeted, I can proceed a with the GHASH changes I
had reviewed back in February.
There are two changes from the previous review. One is the separate
method for the range checking in jdk:GHASH.java in update(). The other
is disabing UseGHASHIntrinsics for arch64 in hotspo
Perfect. No more comments from me.
Thanks,
Vladimir
On 2/20/15 12:58 PM, Anthony Scarpino wrote:
On 02/19/2015 05:36 PM, Vladimir Kozlov wrote:
JDK changes review.
---
All exceptions should print invalid value as you did for state and
subkeyH checks. It would greatly help debug
On 02/19/2015 05:36 PM, Vladimir Kozlov wrote:
JDK changes review.
---
All exceptions should print invalid value as you did for state and
subkeyH checks. It would greatly help debugging.
Please, split first check into 3 separate checks and print invalid value
to know which condit
On Feb 19, 2015, at 5:36 PM, Vladimir Kozlov wrote:
> JDK changes review.
> ---
> All exceptions should print invalid value as you did for state and subkeyH
> checks. It would greatly help debugging.
>
> Please, split first check into 3 separate checks and print invalid value t
JDK changes review.
---
All exceptions should print invalid value as you did for state and
subkeyH checks. It would greatly help debugging.
Please, split first check into 3 separate checks and print invalid value
to know which condition failed.
Missing space after 'length': "
I've updated the webrevs. I believe I've added everyone's comments.
- Changed GHASHIntrinsics and disabled for non-supported platforms
- removing WIN64 ifdef in 32bit x86
- fixed hotspot test so it can use compareArray()
- modified the range checking
- fixed processBlocks comment
http://cr.openj
On 02/17/2015 03:52 PM, Florian Weimer wrote:
On 02/17/2015 11:30 PM, Vladimir Kozlov wrote:
The concern was about java code and not intrinsic. If intrinsic is not
used we will get additional range checks which were not present before.
By indirect load I mean object->array_oop->element instead
Ok, I'll add these checks.. Thanks for looking through this.
Tony
On 02/17/2015 02:30 PM, Vladimir Kozlov wrote:
The concern was about java code and not intrinsic. If intrinsic is not
used we will get additional range checks which were not present before.
By indirect load I mean object->array_
On 02/17/2015 11:30 PM, Vladimir Kozlov wrote:
> The concern was about java code and not intrinsic. If intrinsic is not
> used we will get additional range checks which were not present before.
>
> By indirect load I mean object->array_oop->element instead of
> object->field.
>
> But in blockMult
The concern was about java code and not intrinsic. If intrinsic is not
used we will get additional range checks which were not present before.
By indirect load I mean object->array_oop->element instead of object->field.
But in blockMult() you access them outside main loops. As result the
code
On 02/17/2015 01:14 PM, Vladimir Kozlov wrote:
Florian's concern is valid.
"Range check elimination" means that C2 moves checks from a loop. Checks
are still present. Since 'state' array is not final we can't eliminate
range check.
I don't understand the concern here. The arrays are private a
Florian's concern is valid.
"Range check elimination" means that C2 moves checks from a loop. Checks
are still present. Since 'state' array is not final we can't eliminate
range check. An other thing is an additional indirect load to access
arrays elements.
I would suggest to keep original c
On 02/17/2015 12:05 PM, Florian Weimer wrote:
On 02/17/2015 08:59 PM, Anthony Scarpino wrote:
On 02/17/2015 12:57 AM, Florian Weimer wrote:
On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
http://cr.openjdk.java.net/~ascarpino/8073108/jdk/webrev/
I think the “state” field in GHASH should be f
On 02/17/2015 09:03 PM, Anthony Scarpino wrote:
> On 02/17/2015 02:59 AM, Florian Weimer wrote:
>> On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
>>> Hi,
>>>
>>> I'm requesting a code review to intrinsify the GHASH operations for both
>>> x86 and SPARC platforms. This greatly increases performanc
On 02/17/2015 08:59 PM, Anthony Scarpino wrote:
> On 02/17/2015 12:57 AM, Florian Weimer wrote:
>> On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
>>> http://cr.openjdk.java.net/~ascarpino/8073108/jdk/webrev/
>>
>> I think the “state” field in GHASH should be final. Is C2 able to
>> eliminate the
On 02/17/2015 02:59 AM, Florian Weimer wrote:
On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
Hi,
I'm requesting a code review to intrinsify the GHASH operations for both
x86 and SPARC platforms. This greatly increases performance over
software for AES/GCM crypto operations, details are in the
On 02/17/2015 12:57 AM, Florian Weimer wrote:
On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
http://cr.openjdk.java.net/~ascarpino/8073108/jdk/webrev/
I think the “state” field in GHASH should be final. Is C2 able to
eliminate the array bounds checks? (Although it's not in the inner loop
an
On 02/17/2015 10:41 AM, Vladimir Kozlov wrote:
On 2/17/15 10:31 AM, Anthony Scarpino wrote:
On 02/16/2015 06:01 PM, Vladimir Kozlov wrote:
There is #ifdef _WIN64 in stubGenerator_x86_32.cpp. It is not needed
since it is 32-bit code.
Sure.. Do I still need to save the xmm6 and xmm7 on 32bit co
On 02/16/2015 06:47 PM, John Rose wrote:
Cool stuff. I'm glad to see SPARC xmulx getting some play.
This is not a review, but I have a small and a big comment.
You don't need the argument vmIntrinsics::ID id unless you are going
to do something with it, such as expand one of a family of intrin
On 2/17/15 10:31 AM, Anthony Scarpino wrote:
On 02/16/2015 06:01 PM, Vladimir Kozlov wrote:
On 2/16/15 5:23 PM, David Holmes wrote:
Hi Tony,
Not a review as hotspot compiler folk need to review this.
On 17/02/2015 7:11 AM, Anthony Scarpino wrote:
Hi,
I'm requesting a code review to intrinsi
On 02/16/2015 06:01 PM, Vladimir Kozlov wrote:
On 2/16/15 5:23 PM, David Holmes wrote:
Hi Tony,
Not a review as hotspot compiler folk need to review this.
On 17/02/2015 7:11 AM, Anthony Scarpino wrote:
Hi,
I'm requesting a code review to intrinsify the GHASH operations for both
x86 and SPARC
On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
> Hi,
>
> I'm requesting a code review to intrinsify the GHASH operations for both
> x86 and SPARC platforms. This greatly increases performance over
> software for AES/GCM crypto operations, details are in the bug.
>
> The review is for two repos,
On 02/16/2015 10:11 PM, Anthony Scarpino wrote:
> http://cr.openjdk.java.net/~ascarpino/8073108/jdk/webrev/
I think the “state” field in GHASH should be final. Is C2 able to
eliminate the array bounds checks? (Although it's not in the inner loop
and thus probably not relevant for performance.)
Cool stuff. I'm glad to see SPARC xmulx getting some play.
This is not a review, but I have a small and a big comment.
You don't need the argument vmIntrinsics::ID id unless you are going to do
something with it, such as expand one of a family of intrinsics covered by the
same LCK::inline* rou
On 2/16/15 5:23 PM, David Holmes wrote:
Hi Tony,
Not a review as hotspot compiler folk need to review this.
On 17/02/2015 7:11 AM, Anthony Scarpino wrote:
Hi,
I'm requesting a code review to intrinsify the GHASH operations for both
x86 and SPARC platforms. This greatly increases performance
Hi Tony,
Not a review as hotspot compiler folk need to review this.
On 17/02/2015 7:11 AM, Anthony Scarpino wrote:
Hi,
I'm requesting a code review to intrinsify the GHASH operations for both
x86 and SPARC platforms. This greatly increases performance over
software for AES/GCM crypto operatio
Hi,
I'm requesting a code review to intrinsify the GHASH operations for both
x86 and SPARC platforms. This greatly increases performance over
software for AES/GCM crypto operations, details are in the bug.
The review is for two repos, hotspot and jdk:
http://cr.openjdk.java.net/~ascarpino/8
35 matches
Mail list logo
