[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR 8150388: Remove SPARC 32-bit support
From:       Kim Barrett <kim.barrett () oracle ! com>
Date:       2017-04-21 0:32:12
Message-ID: 9A56C2CB-893E-4759-9882-434B2CA8E6CB () oracle ! com
[Download RAW message or body]

On Apr 19, 2017, at 1:54 PM, George Triantafillou <george.triantafillou@oracle.com> \
wrote:
> 
> Hi Kim,
> 
> Comments inline:
> 
> On 4/15/2017 4:27 AM, Kim Barrett wrote:
> > > On Apr 7, 2017, at 10:47 AM, George Triantafillou \
> > > <george.triantafillou@oracle.com> wrote: 
> > > (Adding compiler).
> > > Please review this fix to remove SPARC 32-bit support.  Support for \
> > > solaris-sparc has been dropped from the list of supported platforms. 
> > > JBS: https://bugs.openjdk.java.net/browse/JDK-8150388
> > > webrev: http://cr.openjdk.java.net/~gtriantafill/8150388-webrev/webrev/ \
> > > <http://cr.openjdk.java.net/%7Egtriantafill/8150388-webrev/webrev/> 
> > > Built and tested on solaris-sparcv9-debug,solaris-x64-debug with the nsk.jvmti, \
> > > nsk.jdwp, and nsk.jdi testlists. 
> > > Thanks.
> > > 
> > > -George
> > That's a lot of files and changes!  I hope you'll be providing
> > incremental webrevs for updates.
> I've checked in the change for 8150388, but will address the issues you raised.

Thanks.

[My email client threaded the message cc'd to the compiler list
separately from the one only to the runtime list where most of the
discussion occurred, so I missed that there had been lots of
other folks looking at this.]

For the places where you said you weren't sure what should be done in
response to my comment, mostly I'm not either. These are places where
I think further cleanup is needed, but it's not mechanical in the way
removing conditionalized dead code was. Rather, someone who actually
understands the code needs to look at it and make appropriate changes.

> > src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
> > 1490       Register yhi = opr2->as_register_hi();
> > 
> > After the removal of the 32bit code, I don't see any remaining
> > references to yhi.  Not sure what that implies, other than there's
> > likely further work to do here.
> It's unclear to me what what should be done here.

Make sure it isn't a bug that yhi is no longer referenced.  Remove if
that's appropriate.  Maybe some variable renaming?

> > src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
> > 2574     Register cmp_value_hi = op->cmp_value()->as_register_hi();
> > ...
> > 2576     Register new_value_hi = op->new_value()->as_register_hi();
> > 
> > After the removal of the 32bit code, I don't see any remaining
> > references to the xxx_hi variables.  Not sure what that implies, other
> > than there's likely further work to do here.
> It's unclear to me what what should be done here.

Like the similar problem at line 1490.

> > src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
> > 3047 void LIR_Assembler::volatile_move_op(LIR_Opr src, LIR_Opr dest, BasicType \
> > type, CodeEmitInfo* info) { 3048   ShouldNotReachHere();
> > 3049
> > 3050   NEEDS_CLEANUP;
> > 
> > It appears that in 64bit mode this function has no implementation, and
> > the following 50+ lines of code are presently 32bit-only.  Likely
> > further work to do here.
> It's unclear to me what what should be done here.

I'm not sure either.  Possibly just delete everything in that function
after the ShouldNotReacheHere(), and comment that line as this not
being needed for _LP64.  That would be consistent with the PPC and
S390 ports.  But x86 and aarch64 ports have code there, and arm port
has an Unimplemented() with a TODO comment for AARCH64.  There may be
code elsewhere that is artificially preventing reaching the
ShouldNotReachHere / Unimplemented cases.  Again, something for
someone who actually understands the code to look at.

> > src/cpu/sparc/vm/globalDefinitions_sparc.hpp
> > 43 #elif defined(COMPILER1)
> > 44   // pure C1, 32-bit, small machine
> > 45   #define DEFAULT_CACHE_LINE_SIZE 16
> > 
> > Comment suggests this is a configuration we don't use with 64bit.
> It's unclear to me what what should be done here.

Just remove the conditionalization and always define as 128? Again,
needs someone who understands the code to look at this.  Note the
default (in globalDefinitions.hpp) is 64.

> > src/cpu/sparc/vm/sparc.ad
> > 314 // 64-bit build means 64-bit pointers means hi/lo pairs
> > 
> > "64-bit build means" seems superfluous now.
> Really?  Suggestions?

Since there are now only 64-bit builds, that part of the comment seems
unnecessary, so just trim it off the front, leaving the rest of the
comment.

> > src/cpu/sparc/vm/sparc.ad
> > 1856 // Note that we if-def off of _LP64.
> > 1857 // The relevant question is how the int is callee-saved.  In _LP64
> > 1858 // the whole long is written but de-opt'ing will have to extract
> > 1859 // the relevant 32 bits, in not-_LP64 only the low 32 bits is written.
> > 
> > The commentary here needs some updating; there is no not-_LP64
> > anymore.
> Agreed. How should it read?

Perhaps something like:

When callee-saving an int value, in _LP64 the whole thing is written,
but de-opt'ing will have to extract the relevant 32 bits.

But again, someone who knows the code should look at it.

> > src/cpu/sparc/vm/templateInterpreterGenerator_sparc.cpp
> > 60 // The sethi() instruction generates lots more instructions when shell
> > 61 // stack limit is unlimited, so that's why this is much bigger.
> > 
> > "why this is much bigger" ? than what?  Oh, the 32bit value.  Some
> > revision of comment is called for here.
> It's from Changeset: 9934 (fd5d53ecf040) 8146410: Interpreter functions are \
> declared and defined in the wrong files.

It’s still confusing, since the referent is to the no longer present 32bit code.

> > src/cpu/sparc/vm/vm_version_sparc.cpp
> > 73     // 32-bit oops don't make sense for the 64-bit VM on sparc
> > 74     // since the 32-bit VM has the same registers and smaller objects.
> > 
> > This comment probably needs updating.
> It's from Changeset: 642 (660978a2a31a) 6791178: Specialize for zero as the \
> compressed oop vm heap base

Yes, but the reference to a 32-bit VM seems orphaned now.  Something should
be changed here, but I don’t know what it should be changed to.


[prev in list] [next in list] [prev in thread] [next in thread]