[aarch64-port-dev ] RFR(M): 8145336: PPC64: fix string intrinsics after CompactStrings change

[Volker Simonis]

Great!
Thanks a lot Vladimir,
Volker


On Wed, Jan 20, 2016 at 6:08 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> +1. Finally UseSSE42Intrinsics was moved!
> I will sponsor it.
>
> Thanks,
> Vladimir
>
>
>
> On 1/20/16 8:45 AM, Doerr, Martin wrote:
>>
>> Hi Volker,
>>
>> thanks for the update. Looks good.
>>
>> Best regards,
>>    Martin
>>
>> -----Original Message-----
>> From: Volker Simonis [mailto:volker.simonis at gmail.com]
>> Sent: Mittwoch, 20. Januar 2016 17:23
>> To: Doerr, Martin <martin.doerr at sap.com>
>> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>;
>> ppc-aix-port-dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8145336: PPC64: fix string intrinsics after
>> CompactStrings change
>>
>> Hi Martin,
>>
>> thanks for your thorough review. I've uploaded a new webrev to:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8145336.v1/
>>
>> Please find my comments inline.
>>
>> Regards,
>> Volker
>>
>>
>> On Wed, Jan 20, 2016 at 12:43 PM, Doerr, Martin <martin.doerr at sap.com>
>> wrote:
>>>
>>> Hi Volker,
>>>
>>> thank you very much for adapting the non-CompactStrings version of the
>>> intrinsics. I especially like that you changed shared code to improve
>>> matching of special cases.
>>>
>>> Here are some minor change requests:
>>> - I guess you will have to adapt Copyright messages.
>>
>>
>> Done.
>>
>>> - There's a typo in the new comment in library_call: "optimzed".
>>
>>
>> Fixed.
>>
>>> - The comment for the instruction count (used for loop alignment) is
>>> wrong in MacroAssembler::string_indexof_1 (should start with 3 instead of
>>> 2).
>>>
>>
>> Right, fixed.
>>
>>> I have more change requests regarding ppc.ad:
>>>
>>> The computation of chr is incorrect for little endian in
>>> string_indexOf_imm1_char and string_indexOf_imm1.
>>>
>>
>> Good catch. Fixed.
>>
>>> Some numbers for compute_padding should be adapted:
>>> int string_indexOf_imm1_charNode::compute_padding(int current_offset)
>>> const { return (3*4-current_offset)&31; }
>>> int string_indexOfCharNode::compute_padding(int current_offset) const {
>>> return (3*4-current_offset)&31; }
>>> int string_compareNode::compute_padding(int current_offset) const {
>>> return (2*4-current_offset)&31; }
>>>
>>
>> Right, and also:
>> int string_indexOf_imm1Node::compute_padding(int current_offset) const
>> { return (3*4-current_offset)&31; }
>>
>> I have now put a comment in each method which points to the  macro
>> assembler method it depends on to make this dependency explicit.
>>
>>> Some kill effects are missing:
>>> - ctr in all string_indexOf nodes
>>
>>
>> Added kill effect for ctr register to all str_indexof intrinsics.
>>
>>> - cr0, cr1 in string_indexOf_imm1, string_indexOfChar
>>>
>>
>> Fixed.
>>
>>> The new comment for string_indexOfChar claims "// Kill ... needle" which
>>> is not true.
>>>
>>
>> Right, fixed.
>>
>>> Thanks,
>>>   Martin
>>>
>>>
>>> -----Original Message-----
>>> From: hotspot-compiler-dev
>>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Volker
>>> Simonis
>>> Sent: Dienstag, 19. Januar 2016 19:57
>>> To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
>>> Cc: ppc-aix-port-dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net
>>> Subject: RFR(M): 8145336: PPC64: fix string intrinsics after
>>> CompactStrings change
>>>
>>> Hi,
>>>
>>> can somebody please review and sponsor this change.
>>>
>>> Despite the bug summary, I still had to do some small shared changes
>>> to make this work, so unfortunately I can not push this on my own.
>>>
>>> The change also affects aarch64 (although it is minimal and I don't
>>> expect it to break anything) so I cc-ed aarch64-port-dev.
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8145336/
>>> https://bugs.openjdk.java.net/browse/JDK-8145336
>>>
>>> As described in the bug, this change only fixes the string intrinsics
>>> for the -XX:-UseCompactStrings mode which is still the default on
>>> ppc64. Additionally, support for the new StrIndexOfChar intrinsic was
>>> added because we already had a similar intrinsic for constant string
>>> needles of length one anyway. A later change (which we're already
>>> working on) will add the intrinsics which can handle compact strings.
>>>
>>> The current intrinsics can handle both, the new byte-array based
>>> string representation as well as the old char-array based string
>>> representation because we internally still use the new hotspot with
>>> older versions of the class libraries.
>>>
>>> I've also ported some of our internal string tests into a new
>>> regression test (TestStringIntrinsics2.java) because the existing
>>> tests didn't exercise all of our intrinsics.
>>>
>>> Following the shared changes I had to do:
>>>
>>> Until now, UseSSE42Intrinsics was a global shared option which was
>>> used to control the availability of the stringIndexOf intrinsics. But
>>> UseSSE42Intrinsics is actually a x86-specific feature so it doesn't
>>> make a lot of sense to define it for other architectures. I've
>>> therefore moved the flag to globals_x86.hpp and changed the condition
>>> which checks for the ability of the stringIndexOf intrinsics from:
>>>
>>> if (!Matcher::has_match_rule(Op_StrIndexOf) || !UseSSE42Intrinsics) {
>>>
>>> to:
>>>
>>> if (!Matcher::match_rule_supported(Op_StrIndexOf)) {
>>>
>>> The Matcher::match_rule_supported() method already calls
>>> Matcher::has_match_rule() anyway. And it is implemented in the .ad
>>> file so I've moved the check for UseSSE42Intrinsics into x86.ad. Other
>>> platforms can now decide in their .ad file if they unconditionally
>>> support the intrinsic or if they need a special feature check. This
>>> change was already briefly discussed in [1].
>>>
>>> The other shared change I had to make was in
>>> LibraryCallKit::make_string_method_node() for the "Op_StrEquals" case.
>>> We have optimized intrinsics for the case that one of the strings to
>>> compare is constant, but the  StrEqualsNode is constructed without
>>> taking into account that one of the string length values could be a
>>> constant. This prevented our optimized instruction from being matched
>>> in the ad-file.
>>>
>>> All the other changes are ppc-specific.
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2015-December/thread.html#20400