review (S) for 6849984: Value methods for platform dependent math functions constant fold incorrectly

[Christian Thalinger]

On Wed, 2010-01-13 at 17:04 -0800, Tom Rodriguez wrote:
> On Jan 13, 2010, at 4:41 PM, Vladimir Kozlov wrote:
> 
> > src/share/vm/runtime/stubRoutines.hpp
> > +   // These are versions of the java.lang.Math methods perform the same
> >                                                       ^ which ?
> > +   // version returns the same result as the the strict version then
> >                                              ^ dup
> > +   // the they can be set to the appropriate function from SharedRuntime.
> 
> Fixed.
> 
> >        ^
> > 
> > src/share/vm/opto/subnode.cpp
> > Did you tested on sparc where all StubRoutines::intrinsic_* are NULL?
> > I don't see their initialization on sparc.
> 
> I ran the whole thing through JPRT.  We don't use those nodes on sparc so that's why they don't have an implementation.  If you use those nodes you are required to provide an implementation.  There's an explicit comment about that in stubGenerator_sparc.cpp.
> 
> > 
> > cpu/x86/vm/stubGenerator_x86_64.cpp
> > Use movdbl() instead of movsd().
> 
> Fixed.
> 
> > 
> > +       StubCodeMark mark(this, "StubRoutines", "cos");
> > ...
> > +       __ movsd(xmm0, Address(rsp, 0));
> > +       __ addq(rsp, 8);
> > +       __ fld_d(Address(rsp, 4)); <<<<<<  What is that???
> 
> I don't know.  It looks like a piece left behind during editing.  I've removed it.

It's a little strange to generate the math intrinsics along with the
arraycopy stubs in:

  void generate_arraycopy_stubs() {

Maybe I missed the point of this change, but why are we using the much
more complicated C versions for these?

+     // The intrinsic version of these seem to return the same value as
+     // the strict version.
+     StubRoutines::_intrinsic_exp = SharedRuntime::dexp;
+     StubRoutines::_intrinsic_pow = SharedRuntime::dpow;

Otherwise this looks good.

-- Christian