RFR(S): 8056950: Compiled code (64-bit) on SPARC should sign extend INT parameters passed on registers to runtime or native methods.

[Lindenmaier, Goetz]

Hi Tobias, 

that's fine, the link to the webrev of 8148353 is in there, anyways.

Best regards,
  Goetz.

> -----Original Message-----
> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
> Sent: Montag, 11. Juli 2016 11:59
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Volker Simonis
> <volker.simonis at gmail.com>
> Cc: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR(S): 8056950: Compiled code (64-bit) on SPARC should sign
> extend INT parameters passed on registers to runtime or native methods.
> 
> Hi Goetz,
> 
> On 11.07.2016 11:42, Lindenmaier, Goetz wrote:
> > Hi Volker,
> >
> > thanks for looking at this.
> > I can't add the bug because I don't have access to it, it's closed.
> > Tobias, can you access it and change it to open?
> 
> Unfortunately, I cannot change it to open because the bug description and
> comments contain confidential information. I still linked the bug.
> 
> > And I can't add the description because jbs doesn't let me log in ...
> > I'll keep trying.
> 
> I saw that you now managed to add a comment. I think that's sufficient for
> other people to understand the limitations of your fix.
> 
> Best regards,
> Tobias
> 
> >
> > Best regards,
> >   Goetz.
> >
> >> -----Original Message-----
> >> From: Volker Simonis [mailto:volker.simonis at gmail.com]
> >> Sent: Montag, 11. Juli 2016 11:13
> >> To: Tobias Hartmann <tobias.hartmann at oracle.com>
> >> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> compiler-
> >> dev at openjdk.java.net
> >> Subject: Re: RFR(S): 8056950: Compiled code (64-bit) on SPARC should sign
> >> extend INT parameters passed on registers to runtime or native methods.
> >>
> >> Hi Goetz,
> >>
> >> your change looks good!
> >>
> >> Can you please also link 8148353 to this bug in JBS. Maybe you can
> >> also add your description from the initial mail to JBS. I think it
> >> could be useful if somebody will look at this again (maybe because he
> >> wants to implement the widening issue).
> >>
> >> @Tobias: thanks for sponsoring.
> >>
> >> Regards,
> >> Volker
> >>
> >>
> >>
> >> On Mon, Jul 11, 2016 at 10:40 AM, Tobias Hartmann
> >> <tobias.hartmann at oracle.com> wrote:
> >>> Hi Goetz,
> >>>
> >>> On 08.07.2016 15:32, Lindenmaier, Goetz wrote:
> >>>> Hi,
> >>>>
> >>>> could I please get a second review?
> >>>> I also please need a sponsor.
> >>>
> >>> I can do the sponsoring once you got a second review.
> >>>
> >>> Best regards,
> >>> Tobias
> >>>
> >>>>
> >>>> Best regards,
> >>>>   Goetz.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
> >>>>> Sent: Mittwoch, 6. Juli 2016 15:37
> >>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> >> compiler-
> >>>>> dev at openjdk.java.net
> >>>>> Subject: Re: RFR(S): 8056950: Compiled code (64-bit) on SPARC should
> >> sign
> >>>>> extend INT parameters passed on registers to runtime or native
> >> methods.
> >>>>>
> >>>>> Hi Goetz,
> >>>>>
> >>>>> On 05.07.2016 12:09, Lindenmaier, Goetz wrote:
> >>>>>> Hi Tobias,
> >>>>>>
> >>>>>> thanks for your comments!
> >>>>>>
> >>>>>> For the widening issue -- it's not straight forward to implement.
> >>>>>> Each intrinsic has it's own section generating the type and call node.
> >>>>>> If these are fixed, it tends to be forgotten if new ones are added.
> >>>>>>
> >>>>>> A generic approach would add the widening further down when
> >>>>>> generating the call, but there e.g. the type can not be changed any
> >>>>>> more because types are constant and it's passed down by reference.
> >>>>>
> >>>>> Right, I agree that this is not easy to fix, so let's leave this for now.
> >>>>>
> >>>>> Best regards,
> >>>>> Tobias
> >>>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>>   Goetz.
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
> >>>>>>> Sent: Dienstag, 5. Juli 2016 09:16
> >>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> >>>>> compiler-
> >>>>>>> dev at openjdk.java.net
> >>>>>>> Subject: Re: RFR(S): 8056950: Compiled code (64-bit) on SPARC
> should
> >> sign
> >>>>>>> extend INT parameters passed on registers to runtime or native
> >> methods.
> >>>>>>>
> >>>>>>> Hi Goetz,
> >>>>>>>
> >>>>>>> thanks for taking care of this bug!
> >>>>>>>
> >>>>>>> I agree that we should think about fixing the C stubs by widening
> ints
> >> to
> >>>>> longs
> >>>>>>> in C2 but as this is currently not an issue, I'm fine with your fix.
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>> Tobias
> >>>>>>>
> >>>>>>> On 04.07.2016 16:35, Lindenmaier, Goetz wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I had a look at "8056950: Compiled code (64-bit) on SPARC should
> >> sign
> >>>>>>> extend INT parameters passed on registers to runtime or native
> >>>>> methods."
> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8056950  We fixed the
> >> same
> >>>>>>> issue for ppc.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> There are three parts to this:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 1.) generate_native_wrapper(). The native wrapper should do
> the
> >> sign
> >>>>>>> extensions for the smaller ints.
> >>>>>>>>
> >>>>>>>> This was missing when 8056950 was opened, but fixed by
> "8148353:
> >>>>> [linux-
> >>>>>>> sparc] Crash in libawt.so on Linux SPARC"
> >>>>>>>>
> >>>>>>>> in the meantime.
> >>>>>>> http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/1f4f4866aee0
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 2.)  Stubs called directly like updateBytesCRC32C. If the C2
> compiler
> >>>>> passes
> >>>>>>> an int to these it is not
> >>>>>>>>
> >>>>>>>> correctly sign extended. updateBytesCRC32C and most others are
> >>>>>>> implemented in assembly, so one can
> >>>>>>>>
> >>>>>>>>  assure proper 32-bit instructions are used.
> >>>>>>>>
> >>>>>>>> A problem are stubs implemented in C, as montgomery_square()
> on
> >>>>> ppc.
> >>>>>>> On ppc we solved this with
> >>>>>>>>
> >>>>>>>>  a trick in the platform implementations, see
> >>>>> sharedRuntime_ppc.cpp:3445,
> >>>>>>> "len = len & 0x7fffFFFF;"
> >>>>>>>>
> >>>>>>>> Montgomery is not implemented on sparc, though. I did not find
> any
> >>>>> other
> >>>>>>> similar on sparc.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 3.) The optoStubs generated by the C2 compiler. The C2 compiler
> will
> >>>>> insert
> >>>>>>> proper casts here if
> >>>>>>>>
> >>>>>>>>   CCallingConventionRequiresIntsAsLongs is set. So this needs to
> be
> >> set
> >>>>> on
> >>>>>>> sparc.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I propose the folowing simple, straight forward webrev:
> >>>>>>>>
> >>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8056950-
> >>>>>>> SparcIntToLong/webrev.01/
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> For 2.)/montgtomery, one could think about implementing
> widening
> >> the
> >>>>>>> type of 'len' to
> >>>>>>>>
> >>>>>>>> long in C2, but it's currently not an issue.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>>
> >>>>>>>>   Goetz.
> >>>>>>>>