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

[Vladimir Kozlov]

+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