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

[Bhateja, Jatin]

Hi Bernard, 

Thanks, let me if there are other comments / result of your test runs.
Will then send the updated patch.

Regards,
Jatin 

> -----Original Message-----
> From: B. Blaser [mailto:bsrbnd at gmail.com]
> Sent: Wednesday, February 6, 2019 4:34 PM
> To: Bhateja, Jatin <jatin.bhateja at intel.com>
> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, Sandhya
> <sandhya.viswanathan at intel.com>; hotspot-compiler-
> dev at openjdk.java.net; Deshpande, Vivek R <vivek.r.deshpande at intel.com>
> Subject: Re: [PATCH] 8217561 : X86: Add floating-point Math.min/max
> intrinsics
> 
> Hi Jatin,
> 
> On Mon, 4 Feb 2019 at 19:57, Bhateja, Jatin <jatin.bhateja at intel.com>
> wrote:
> >
> > Hi All,
> >
> > Please find the link to updated patch.
> >
> > http://cr.openjdk.java.net/~sviswanathan/Jatin/8217561/webrev.02/
> >
> > It contains fix to stop unnecessary register to register copy when
> > source and destination are same, test case updated to reflect correct
> copyright year and has been tested over both 32 and 64 bit variants.
> >
> > Regards,
> > Jatin
> 
> I like your patch and I guess it works but I still disapprove the test case
> parameters.
> 
> Try this:
> 
> diff --git a/src/hotspot/share/opto/library_call.cpp
> b/src/hotspot/share/opto/library_call.cpp
> --- a/src/hotspot/share/opto/library_call.cpp
> +++ b/src/hotspot/share/opto/library_call.cpp
> @@ -6640,6 +6640,7 @@
>    default:  fatal_unexpected_iid(id);  break;
>    }
>    set_result(_gvn.transform(n));
> +printf("#### inline_fp_min_max ####\n");
>    return true;
>  }
> 
> or add 'printf()' in:
> 
> void Assembler::vmaxpd(XMMRegister dst, XMMRegister nds, XMMRegister
> src, int vector_len) { printf("#### vmaxpd ####\n");
> 
> and run the test case:
> 
> $ make test
> TEST="jtreg:test/hotspot/jtreg/compiler/intrinsics/math/TestFpMinMaxIntrin
> sics.java"
> 
> Then, check the test report:
> 
> command: main -XX:+UnlockDiagnosticVMOptions -Xcomp -XX:-
> TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -
> XX:CompileOnly=java/lang/Math
> compiler.intrinsics.math.TestFpMinMaxIntrinsics
> reason: User specified action: run main/othervm -
> XX:+UnlockDiagnosticVMOptions -Xcomp -XX:-TieredCompilation -
> XX:+IgnoreUnrecognizedVMOptions -XX:CompileOnly=java/lang/Math
> compiler.intrinsics.math.TestFpMinMaxIntrinsics
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 0.671
> ----------configuration:(0/0)----------
> ----------System.out:(0/0)----------
> ----------System.err:(1/15)----------
> STATUS:Passed.
> 
> We don't see any 'printf()' meaning that the intrinsic isn't inlined!
> 
> Now that you added enough iterations in your main loop (>10K as Vladimir
> said), add the following line to the test case:
> 
>  * @run main/othervm  compiler.intrinsics.math.TestFpMinMaxIntrinsics
> 
> rerun the test and check again the report:
> 
> command: main compiler.intrinsics.math.TestFpMinMaxIntrinsics
> reason: User specified action: run main/othervm
> compiler.intrinsics.math.TestFpMinMaxIntrinsics
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 0.068
> ----------configuration:(0/0)----------
> ----------System.out:(24/584)----------
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### vmaxpd ####
> ----------System.err:(1/15)----------
> STATUS:Passed.
> 
> The intrinsic is clearly inlined now.
> 
> As I said earlier, you have to enable the compilation of the caller
> (TestFpMinMaxIntrinsics::dTest) to inline the intrinsic in its generated code
> and I guess this is what Nils meant.
> The problem with the current parameters is that '-
> XX:CompileOnly=java/lang/Math' prevents the compilation of
> 'TestFpMinMaxIntrinsics::dTest', so simply try to replace it with:
> 
> -XX:CompileOnly=compiler.intrinsics.math.TestFpMinMaxIntrinsics::dTest
> 
> and rerun the test:
> 
> command: main -XX:+UnlockDiagnosticVMOptions -Xcomp -XX:-
> TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -
> XX:CompileOnly=compiler.intrinsics.math.TestFpMinMaxIntrinsics::dTest
> compiler.intrinsics.math.TestFpMinMaxIntrinsics
> reason: User specified action: run main/othervm -
> XX:+UnlockDiagnosticVMOptions -Xcomp -XX:-TieredCompilation -
> XX:+IgnoreUnrecognizedVMOptions -
> XX:CompileOnly=compiler.intrinsics.math.TestFpMinMaxIntrinsics::dTest
> compiler.intrinsics.math.TestFpMinMaxIntrinsics
> Mode: othervm [/othervm specified]
> elapsed time (seconds): 0.634
> ----------configuration:(0/0)----------
> ----------System.out:(8/180)----------
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### vmaxpd ####
> #### vmaxpd ####
> #### inline_fp_min_max ####
> #### inline_fp_min_max ####
> #### vmaxpd ####
> #### vmaxpd ####
> ----------System.err:(1/15)----------
> STATUS:Passed.
> 
> The intrinsic is also inlined!
> 
> But, playing with these parameters is tricky as Vladimir said, so I suggest you
> simply add:
> 
>  * @run main/othervm  compiler.intrinsics.math.TestFpMinMaxIntrinsics
> 
> with a loop of more than 10K iterations.
> 
> Got it?
> 
> I'll run test-tier1 on x86_64 (xeon) but I don't work anymore on x86_32, so I
> let Oracle comment on this part as I'm not sure how much do they still use it?
> 
> Regards,
> Bernard