RFR(M): 8145336: PPC64: fix string intrinsics after CompactStrings change

[Doerr, Martin]

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