February 2019 - aarch64-port-dev

RFR: 8218748: AARCH64: String::compareTo intrinsic documentation and maintenance improvement
by Dmitrij Pochepko 18 Nov '19

18 Nov '19

Hi all. Please review an enhancement change "8218748: AARCH64: String::compareTo intrinsic documentation and maintenance improvement". RFE: https://bugs.openjdk.java.net/browse/JDK-8218748 webrev: http://cr.openjdk.java.net/~dpochepk/8218748/webrev/ Changes: - Added documentation for compareTo implementation. - Removed duplicated code and using existing code instead. -- compare_string_16_bytes_same is used for tail handling in generate_compare_long_string_same_encoding . -- DIFF block code is reused for DIFF2 block in generate_compare_long_string_same_encoding. - Registers and labels are partially renamed to better reflect code actions. This patch is dependent on JDK-8218966 and should be applied on top of it. Testing: - jck - hotspot jtreg compiler/*, runtime/* and gc/* tests. Including AARCH64-implementation-specific compareTo tests introduced in JDK-8218966 fix - jdk jtreg tier1-3 tests No regressions were found. I'd like to thank Pengfei Li (Pengfei.Li(a)arm.com) for pre-review and additional testing. Thanks, Dmitrij

5 10

AARCH64 support for lworld
by Dmitry Samersoff 08 Sep '19

08 Sep '19

Hello Everybody, I started implementation of AARCH64 support for lworld. I'm on very early stage so any advices are highly appreciated. Should I file a CR for this task somewhere? What is correct way to do it? -Dmitry -- Dmitry Samersoff http://devnull.samersoff.net * There will come soft rains ...

6 28

[Roland Westrelin] Re: Aarch64 port for ZGC, so far
by Roland Westrelin 24 May '19

24 May '19

Just noticed I didn't include the mailing lists in that reply. Roland.

7 43

8218966: AArch64: String.compareTo() can read memory after string
by Dmitrij Pochepko 20 May '19

20 May '19

Hi all, Please review a fix for "8218966: AArch64: String.compareTo() can read memory after string". bug: https://bugs.openjdk.java.net/browse/JDK-8218966 webrev: http://cr.openjdk.java.net/~dpochepk/8218966/webrev/ Intrinsic implementation returns wrong value in rare cases for strings longer than 72 characters. Changes: - Different encodings case. Small 16-characters loop and post-loop code are re-organized to stop at string end. Post-loop now also uses compare_string_16_x_LU() to avoid code duplication. - Changed calculation of prefetchLoopExitCondition. It might be incorrect in case when SoftwarePrefetchHintDistance was set to non-default small value. - Same encoding case. Moved loop counter update out of prefetch block. It might miss end-of-string check when prefetch is disabled, with memory after string being read. - Added 2 tests. They are quite similar but TestStringCompareToSameLength compares strings of same length, and TestStringCompareToDifferentLength is for different lengths. Tests cover 8218966 case. And also they cover different parts of intrinsic, taking into account conditions in the implementation and possible SoftwarePrefetchHintDistance values. Testing: Existing jtreg and jck tests were not able to detect 8218966 case. But they pass with the fix applied. Newly added jtreg tests can detect the issue and potential problems in case of changes in the implementation. The following testing was performed: - jck with default vm flags - jck with -Xcomp -XX:-TieredCompilation - hotspot jtreg tests (including new tests): compiler/*, runtime/*, gc/* with default vm flags - hotspot jtreg tests (including new tests): compiler/*, runtime/*, gc/* with -Xcomp -XX:-TieredCompilation - jdk jtreg tier1-3 tests with default vm flags - jdk jtreg tier1-3 tests with -Xcomp -XX:-TieredCompilation No regressions were found. I'd like to thank Pengfei Li (Pengfei.Li(a)arm.com) for pre-review and additional testing. I'm also about to send separate additional webrev with compareTo intrinsic documentation and maintenance-related improvements as separate enhancement. Thanks, Dmitrij

2 1

Re: [aarch64-port-dev ] RFR(S): 8215792: AArch64: String.indexOf generates incorrect result
by Pengfei Li (Arm Technology China) 19 May '19

19 May '19

Hi Dmitrij, Thanks a lot for your reply. > since cnt2 is used as counter, wouldn't it be easier and shorter just to substract cnt1 from cnt2 at the beginning of this code. Total (cnt2 - cnt1 +1) combinations must be checked. That is why first sustraction is by (wordSize/str2_chr_size - 1). > Then whole fix will be probably just 1 line at the beginning: sub(cnt2, cnt2, cnt1); I don't think the whole fix could be as easy as "sub(cnt2, cnt2, cnt1)" because cnt2 is the counter which counts number of bytes not processed. It could be different from the number of bytes after current first-character-match index. But this is just my thought. Perhaps I didn't understand your idea and code thoroughly. So could you post your shorter fix and let's test if it's right? -- Thanks, Pengfei

6 14

RFR: RFR: Bulk integration shenandoah/jdk8u -> aarch64-port/jdk8u-shenandoah 2018-12-13
by Roman Kennke 29 Apr '19

29 Apr '19

Please review the following proposed changes: This integrates changes from shenandoah/jdk8u into aarch64-port/jdk8u-shenandoah repository, reaching back to September 2018. It's bugfixes, improvements and new features that have been originally done in Shenandoah dev repo, and then backported to Shenandoah JDK8u repo and tested there for a while too. List of changes: http://cr.openjdk.java.net/~rkennke/jdk8u-shenandoah-integration-2018-12-13… Webrev showing only shared-changes: http://cr.openjdk.java.net/~rkennke/jdk8u-shenandoah-integration-2018-12-13… Full webrev: http://cr.openjdk.java.net/~rkennke/jdk8u-shenandoah-integration-2018-12-13… Thanks, Roman

5 15

RFR: 8217368: AArch64: C2 recursive stack locking optimisation not triggered
by Nick Gasson (Arm Technology China) 03 Apr '19

03 Apr '19

Hi, While I was cleaning up the patch for 8216350 I noticed an issue in the implementation of recursive locking in aarch64_enc_fast_lock: Bug: https://bugs.openjdk.java.net/browse/JDK-8217368 Webrev: http://cr.openjdk.java.net/~ngasson/8217368/webrev.0/ First we load the markOop of the object we want to lock and OR it with markOopDesc::unlocked_value (1). Then we do a CAS to exchange the address of the box on our thread's stack with the object's header word iff it's equal to the (markOop | 1) we just computed. If this fails, then we should check for a recursive lock by comparing (~(page size - 1) | 3) & (markOop - SP) == 0 Where "markOop" is the current object header word loaded by the failed CAS. This checks that the lock bits are zero (locked) and the stack address of the displaced header is within one page of the current SP. But on AArch64 we actually do this: (~(page size - 1) | 3) & ((old markOop | 1) - SP) == 0 Where "old markOop | 1" is the compare-to value used for the CAS. This is always false as the result has at least bit #0 set. This only affects C2, the C1_MacroAssembler version has the correct test. The diff looks big but all it does is swap the usage of registers `tmp' and `disp_hdr' in the first section so the markOop loaded by the CAS ends up in disp_hdr and tmp holds the (markOop | 1) compare-to value. Ran jtreg, plus jcstress with -XX:+UseLSE and -XX:-UseLSE. Also added another microbenchmark to micro/org/openjdk/bench/vm/lang/LockUnlock.java as I couldn't find an existing JMH case that triggered this. Without patch: Result "org.openjdk.bench.vm.lang.LockUnlock.testRecursiveSynchronizationNoBias": 510.781 ?(99.9%) 1.196 ns/op [Average] (min, avg, max) = (508.769, 510.781, 513.854), stdev = 1.597 CI (99.9%): [509.585, 511.977] (assumes normal distribution) With patch: Result "org.openjdk.bench.vm.lang.LockUnlock.testRecursiveSynchronizationNoBias": 197.038 ?(99.9%) 0.096 ns/op [Average] (min, avg, max) = (196.886, 197.038, 197.296), stdev = 0.128 CI (99.9%): [196.942, 197.134] (assumes normal distribution) Two other minor things: * Does anyone know what the comment "// Load Compare Value application register." means? It's present in the PPC and S390 ports too. * The x86 port #ifdef LP64 uses "7 - os::vm_page_size()" as the mask in the recursive lock test. I think the "7" here is markOopDesc::biased_lock_mask and is presumably there to prevent a silent mutual exclusion failure if a markOop with the bias locking bits set ends up the fast_lock path (although this should never happen). Should we change markOopDesc::lock_mask_in_place to markOopDesc::biased_lock_mask_in_place in the AArch64 port too? Thanks, Nick

4 14

RFR(S): 8214922: Add vectorization support for fmin/fmax
by Pengfei Li (Arm Technology China) 12 Mar '19

12 Mar '19

Hi, JBS: https://bugs.openjdk.java.net/browse/JDK-8214922 Webrev: http://cr.openjdk.java.net/~pli/rfr/8214922/webrev.00/ This is the vectorization support of floating-point min/max. Both vector binary operations and reduction operations are added. For C2 vectorization nodes, I see similar code in project panama (http://hg.openjdk.java.net/panama/dev/) So to reduce duplicate work and help future code merge between jdk/jdk and panama/dev, most of the hotspot shared codes in this patch are copied from panama/dev. I still made one thing different from panama code. It is that for reduction min/max, scalar operations are transformed only when the number of lanes in the vector is greater than 2. I did this as on some architectures, such as AArch64, there is no reduction min/max for 2 floats or 2 doubles. We can surely add a fallback matching rule in ad file for these cases just like reduce_add2F or reduce_add2D in aarch64.ad. But based on my test[1], the fallback implementations show worse performance than the original scalar version. Please let me know if you agree with this change or not. [1] http://cr.openjdk.java.net/~pli/rfr/8214922/TestSIMDFpMinMax.java -- Thanks, Pengfei

6 22

Re: [aarch64-port-dev ] [PATCH] 8217561 : X86: Add floating-point Math.min/max intrinsics, approval request
by Vladimir Kozlov 11 Mar '19

11 Mar '19

Hi Jatin, I am fine with your additional changes but it rises the issue of platform dependency. New code to check probability and your check seems important only for x86. So I have question for aarch64 developers. Are aarch64 fmin/fmax instructions are always faster than code generated by default? If this is true new conditions should be x86 specific. To have a separate function to do these checks. We have precedent - clear_upper_avx(). May be later we have to add other conditions for other platforms too. Thanks, Vladimir On 2/27/19 9:22 AM, Bhateja, Jatin wrote: > Hi Bernard, > I have few queries, please find them embedded in below mail. > > Best Regards, > Jatin > >> -----Original Message----- >> From: B. Blaser [mailto:bsrbnd@gmail.com] >> Sent: Wednesday, February 27, 2019 10:13 PM >> To: Vladimir Kozlov <vladimir.kozlov(a)oracle.com> >> Cc: Andrew Haley <aph(a)redhat.com>; Andrew Dinn <adinn(a)redhat.com>; >> hotspot-compiler-dev(a)openjdk.java.net; Bhateja, Jatin >> <jatin.bhateja(a)intel.com>; Viswanathan, Sandhya >> <sandhya.viswanathan(a)intel.com> >> Subject: Re: [PATCH] 8217561 : X86: Add floating-point Math.min/max >> intrinsics, approval request >> >> On Tue, 26 Feb 2019 at 22:34, Vladimir Kozlov <vladimir.kozlov(a)oracle.com> >> wrote: >>> >>> On 2/26/19 1:14 PM, B. Blaser wrote: >>>> Hi Vladimir, >>>> >>>> On Tue, 26 Feb 2019 at 21:50, Vladimir Kozlov >>>> <vladimir.kozlov(a)oracle.com> wrote: >>>>> >>>>> Hi Bernard, >>>>> >>>>> Unfortunately HotSpot does not collect profiling information per call >> site. >>>>> Currently calee's data is collected from all call sites. For >>>>> example, you can have 2 calls with same *swapped* arguments and you >> will get your balance == 0. >>>>> I would assume if you have multiply call sites in application the >>>>> profiling data will be "flat" - balance value will be near 0. >>>>> >>>>> We did experiment few years ago to collect data per call site but >>>>> we got negative result - no significant improvement in code quality in >> cost of a lot more memory used for MethodData. >>>>> But your case (and cmove) is special since MDO is small for these >>>>> methods. The only downside is to implement that you will need a lot >> more complex changes in HotSpot. >>>>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8015416 >>>>> >>>>> Note, your code is correct in a sense that it use the same >>>>> profiling data as it currently used for inlining these methods. So I don't >> want to reject changes from the start. >>>>> >>>>> The only simple solution I can think of is to use intrinsic only >>>>> for hottest call site. Compare invocation_count with caller's call site >> count to find if it is most used site which contributed the most profiling data. >>>>> >>>>> Thanks, >>>>> Vladimir >>>> >>>> Thanks for your feedback. >>>> >>>> I pushed webrev.05 to jdk/submit a while ago but once I have the >>>> results I'll update the patch as you suggested; the heuristic will >>>> bail out if the caller didn't contribute enough to the statistics, >>>> let say something like at least 80%? >>> >>> Yes, this sounds good. >>> >>> Vladimir >> >> Done: http://hg.openjdk.java.net/jdk/submit/rev/d164e0b595e6 >> >> Please let me know if you've any more comments or if I can push this to the >> main-line (jdk/jdk) once I've got positive test results? > > Profile based intrinsification will not be able to take care of following:- > > 1) De-optimization scenario : Under what condition will this method de-optimize if future inputs change the initially computed balancing factor. > 2) How will balancing factor change with each invocation of C2 since MDO / profile is not affected by code generated by C2. > 3) Check to de-optimize should be part of generated code, but it would add extra cost, in addition C2 does not alter the profiles as of now. > > Vladimir's mail already cleared some of above concerns. > > Another case which was mentioned over the thread sometime back was about reduction which will show better performance with current implementation(non-intrinsic version). > > for (int I = 0 ; I < ITERS ; I++) > Res = Math.max(Res, arr[i]); > > Cross iteration dependency coupled with inter statement RAW dependency within new monolithic sequence of instructions will degrade the performance in reduction scenario. > > Addition check to prevent creation of intrinsic node (Max/Min[FD]Node) in LibraryCallKit::inline_fp_min_max something on following lines will avoid any degradation in reduction > scenarios. > > --- a/src/hotspot/share/opto/library_call.cpp Tue Feb 26 14:57:23 2019 +0530 > +++ b/src/hotspot/share/opto/library_call.cpp Wed Feb 27 20:13:01 2019 +0530 > @@ -6632,6 +6632,12 @@ > if (a->is_Con() || b->is_Con()) { > return false; > } > +#ifdef X86 > + // Being conservative since all the phi edges may not be set > + // by now. This is done to skip over reduction scenarios. > + if (a->is_Phi() || b->is_Phi()) > + return false; > +#endif > > Regards, > Jatin > > >> >> Thanks, >> Bernard

6 21

Re: [aarch64-port-dev ] RFR (trivial): 8219519: Remove linux_sparc.ad and linux_aarch64.ad
by Pengfei Li (Arm Technology China) 06 Mar '19

06 Mar '19

Hi Jie, Tobias, I (Not-A-Reviewer) tested this change on AArch64. Test result shows "make images" is successful and there's no new jtreg failures after linux_aarch64.ad is removed. But I don't have confidence to say this change has no other side effect. -- Thanks, Pengfei > -----Original Message----- > From: hotspot-compiler-dev <hotspot-compiler-dev- > bounces(a)openjdk.java.net> On Behalf Of Jie Fu > Sent: Wednesday, February 27, 2019 10:25 > To: Tobias Hartmann <tobias.hartmann(a)oracle.com>; hotspot-compiler- > dev(a)openjdk.java.net compiler <hotspot-compiler-dev(a)openjdk.java.net> > Subject: Re: RFR (trivial): 8219519: Remove linux_sparc.ad and > linux_aarch64.ad > > Hi Tobias, > > Thanks a lot for your review. > > It's a bit difficult for me to test this patch since I don't have a sparc or arm > machine. > I've analyzed the adlc processing logic in > make/hotspot/gensrc/GensrcAdlc.gmk finding that ad-files under > ./src/hotspot/os_cpu/$(HOTSPOT_TARGET_OS)_$(HOTSPOT_TARGET_CPU_ > ARCH) are optional. > Since both linux_sparc.ad and linux_aarch64.ad are useless for the > generation of C2, it would be better to remove them. > > I'll try my best to test it. > > By the way, I really appreciate If someone with sparc or aarch64 > development environment could help to verify this change. > Thanks in advance. > > Best regards, > Jie > > > On 2019/2/27 上午12:51, Tobias Hartmann wrote: > > Hi Jie, > > > > this looks good to me assuming that you have tested the change on these > platforms. > > > > Best regards, > > Tobias > > > > On 21.02.19 10:35, Jie Fu wrote: > >> Hi all, > >> > >> The following two source files are useless for the generation of C2 and > should be removed. > >> 1) ./src/hotspot/os_cpu/linux_sparc/linux_sparc.ad > >> 2) ./src/hotspot/os_cpu/linux_aarch64/linux_aarch64.ad > >> > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8219519 > >> Webrev: http://cr.openjdk.java.net/~jiefu/8219519/webrev.00/ > >> > >> Could you please review it? > >> Thanks a lot. > >> > >> Best regards, > >> Jie > >> > >>

4 9