RFR 8150388: Remove SPARC 32-bit support
George Triantafillou
george.triantafillou at oracle.com
Wed Apr 19 17:54:08 UTC 2017
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 at 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.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
> 1393 // the poll may need a register so just pick one that isn't the return register
>
> This comment appears to refer to the 32bit code that followed it and
> was removed. I think the comment should be removed too.
Agreed.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
> 1559 } else
> 1560 __ brx(acond, false, Assembler::pt, skip); // checks icc on 32bit and xcc on 64bit
>
> Please add braces to that else-clause. They were missing before
> because the if/else was 64bit-only, and the close brace would have
> also needed 64bit conditionalization.
Agreed.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
> rc/cpu/sparc/vm/copy_sparc.hpp
> 116 static void pd_conjoint_jlongs_atomic(jlong* from, jlong* to, size_t count) {
>
> --- TBD
> Removed call to _Copy_conjoint_jlongs_atomic. Check if it's still
> needed elsewhere or definition is removed.
It's used elsewhere.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/globals_sparc.hpp
> 59 // Stack slots are 2X larger in LP64 than in the 32 bit VM.
>
> Comment seems dead now.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/macroAssembler_sparc.inline.hpp
>
> There are a lot of operations here that are for dispatching on 32/64.
> Some of them may be worth keeping for documentation purposes, but some
> might be better off being removed. That kind of cleanup should
> probably wait for a later RFE though.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/macroAssembler_sparc.inline.hpp
> 323 inline intptr_t MacroAssembler::load_pc_address( Register reg, int bytes_to_skip ) {
> 324 intptr_t thepc = (intptr_t)pc() + 2*BytesPerInstWord + bytes_to_skip;
>
> 325 Unimplemented();
>
> Presumably never called in 64bit mode.
Agreed.
>
> ------------------------------------------------------------------------------
> 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?
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/sparc.ad
> 355 // 32-bit, longs in 1 register: cannot use I's and L's. Restrict to O's and G's.
>
> Comment seems superfluous now.
Agreed.
>
> ------------------------------------------------------------------------------
> 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?
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/sparc.ad
> 1947 // The intptr_t operand types, defined by textual substitution.
> 1948 // (Cf. opto/type.hpp. This lets us avoid many, many other ifdefs.)
>
> These comments suggest there are further cleanup opportunities,
> probably for a later RFE.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> Removed block starting with:
> 831 #if defined(ASSERT) && defined(_LP64)
>
> That looks like an incorrect removal.
The checked in change looks like this:
779 void assert_clean_int(Register Rint, Register Rtmp) {
780 #if defined(ASSERT)
781 __ signx(Rint, Rtmp);
782 __ cmp(Rint, Rtmp);
783 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
784 #endif
785 }
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1232 if (!aligned)
>
> 1233 {
>
> Please move the { to line 1232.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1334 } else
>
> 1335 {
>
> Please move the { to line 1334.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1445 if (!aligned)
>
> 1446 {
>
> Please move the { to line 1445.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1562 if (!aligned) {
>
> 1563 // align to 8 bytes, we know we are 4 byte aligned to start
> 1564 __ andcc(to, 7, G0);
> 1565 __ br(Assembler::zero, false, Assembler::pt, L_fill_32_bytes);
> 1566 __ delayed()->nop();
> 1567 __ stw(value, to, 0);
> 1568 __ inc(to, 4);
> 1569 __ dec(count, 1 << shift);
> 1570 __ BIND(L_fill_32_bytes);
>
> 1571 }
>
> Please fix the indentation.
Agreed.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1777 } else
>
> 1778 {
>
> Please move the { to line 1777.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 1887 if (!aligned)
>
> 1888 {
>
> Please move the { to line 1887.
>
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 3085 } else
>
> 3086 {
>
> Please move the { to line 3085.
Agreed.
> ------------------------------------------------------------------------------
> src/cpu/sparc/vm/stubGenerator_sparc.cpp
> 5103 // Write the high part of the address
> 5104 // [RGV] Check if there is a dependency on the size of this prolog
>
> Pre-existing: These comments seem mis-indented. I wonder who RGV is,
> and how old this comment is.
Agreed. I'll fix the indentation. It's a very old comment.
>
> ------------------------------------------------------------------------------
> 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.
>
> ------------------------------------------------------------------------------
> 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
>
> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.hpp
> 166 #if defined(COMPILER2) || defined(_LP64)
> and
> 228 #endif // _LP64 || COMPILER2
>
> I think the _LP64 parts of these can be removed.
The checked in change addresses this.
>
> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.hpp
> 57 #ifdef _GNU_SOURCE
>
> For a future clenaup, Solaris Studio now supports the gcc-style inline
> assembler being protected by this #ifdef. We could probably eliminate
> the alternative definitions, though if we're not already using the
> inline versions they will need to be carefully examined first.
Agreed.
>
> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/prefetch_solaris_sparc.inline.hpp
> 30 #if defined(COMPILER2) || defined(_LP64)
> and
> 58 #endif // defined(COMPILER2) || defined(_LP64)
>
> I think the _LP64 parts of these can be removed.
The checked in change addresses this.
>
> ------------------------------------------------------------------------------
>
More information about the hotspot-compiler-dev
mailing list