[PATCH] 8217561 : X86: Add floating-point Math.min/max intrinsics, approval request

[Bhateja, Jatin]

Hi Bernard,
I have few queries, please find them embedded in below mail.

Best Regards,
Jatin

> -----Original Message-----
> From: B. Blaser [mailto:bsrbnd at gmail.com]
> Sent: Wednesday, February 27, 2019 10:13 PM
> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Cc: Andrew Haley <aph at redhat.com>; Andrew Dinn <adinn at redhat.com>;
> hotspot-compiler-dev at openjdk.java.net; Bhateja, Jatin
> <jatin.bhateja at intel.com>; Viswanathan, Sandhya
> <sandhya.viswanathan at 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 at 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 at 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