RFR 8150388: Remove SPARC 32-bit support

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8150388: Remove SPARC 32-bit support

George Triantafillou

(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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

George Triantafillou
Thanks Harold, I've made the suggested changes.

-George

On 4/7/2017 10:34 AM, harold seigel wrote:

> Hi George,
>
> Most of the changes look good.  A few comments:
>
> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
> just the "&& defined(_LP64)"
>
>    831 #if defined(ASSERT) && defined(_LP64)
>    832 __ signx(Rint, Rtmp);
>    833 __ cmp(Rint, Rtmp);
>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>    835 #endif
>
> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>
> 3. In prefetch_solaris_sparc.inline, remove lines 30 and 63
>
> 4. In thread_solaris_sparc.hpp, remove line 67.
>
> Thanks, Harold
>
> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>> 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Gerald Thornbrugh
In reply to this post by George Triantafillou
Hi George,

Wow, this is bigger than I thought it would be.

A couple of questions:

I assume you will update the copyright years before you putback?


Line 245, you can probably remove the " || defined(_LP64)”
Line 372, you can probably remove the "_LP64 ||”

Is there a reason you did not include prefetch_solaris_sparc.inline.hpp in your changes?


Line 30, 58 and 63, you can probably remove the references to _LP64

Is there a plan to remove the SPARC 32 bit support from the makefiles and build configuration files?

Thanks!

Jerry


On Apr 7, 2017, at 8:47 AM, George Triantafillou <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Vladimir Kozlov
In reply to this post by George Triantafillou
On 4/7/17 8:04 AM, George Triantafillou wrote:

> Thanks Harold, I've made the suggested changes.
>
> -George
>
> On 4/7/2017 10:34 AM, harold seigel wrote:
>> Hi George,
>>
>> Most of the changes look good.  A few comments:
>>
>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>> just the "&& defined(_LP64)"
>>
>>    831 #if defined(ASSERT) && defined(_LP64)
>>    832 __ signx(Rint, Rtmp);
>>    833 __ cmp(Rint, Rtmp);
>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>    835 #endif

I still see this code is removed. Did you updated webrev?

>>
>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>
>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63

Yes. We don't build JVM with C1 only for 64 bit.

>>
>> 4. In thread_solaris_sparc.hpp, remove line 67.

Yes.

Other compiler code changes seems fine.

Thanks,
Vladimir

>>
>> Thanks, Harold
>>
>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>> 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
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Vladimir Kozlov
Hi George,

I noticed that you did not changed code guarded by NOT_LP64() and
LP64_ONLY() macros.

Also the code in src/os_cpu/linux_sparc/vm is not changed.

Vladimir

On 4/7/17 10:26 AM, Vladimir Kozlov wrote:

> On 4/7/17 8:04 AM, George Triantafillou wrote:
>> Thanks Harold, I've made the suggested changes.
>>
>> -George
>>
>> On 4/7/2017 10:34 AM, harold seigel wrote:
>>> Hi George,
>>>
>>> Most of the changes look good.  A few comments:
>>>
>>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>>> just the "&& defined(_LP64)"
>>>
>>>    831 #if defined(ASSERT) && defined(_LP64)
>>>    832 __ signx(Rint, Rtmp);
>>>    833 __ cmp(Rint, Rtmp);
>>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>>    835 #endif
>
> I still see this code is removed. Did you updated webrev?
>
>>>
>>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>>
>>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63
>
> Yes. We don't build JVM with C1 only for 64 bit.
>
>>>
>>> 4. In thread_solaris_sparc.hpp, remove line 67.
>
> Yes.
>
> Other compiler code changes seems fine.
>
> Thanks,
> Vladimir
>
>>>
>>> Thanks, Harold
>>>
>>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>>> 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
>>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

George Triantafillou
In reply to this post by Vladimir Kozlov
Hi Vladimir,

Thanks for your comments.

Updated webrev:
http://cr.openjdk.java.net/~gtriantafill/8150388-webrev/webrev.01/ 
<http://cr.openjdk.java.net/%7Egtriantafill/8150388-webrev/webrev.01/>

-George

On 4/7/2017 1:26 PM, Vladimir Kozlov wrote:

> On 4/7/17 8:04 AM, George Triantafillou wrote:
>> Thanks Harold, I've made the suggested changes.
>>
>> -George
>>
>> On 4/7/2017 10:34 AM, harold seigel wrote:
>>> Hi George,
>>>
>>> Most of the changes look good.  A few comments:
>>>
>>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>>> just the "&& defined(_LP64)"
>>>
>>>    831 #if defined(ASSERT) && defined(_LP64)
>>>    832 __ signx(Rint, Rtmp);
>>>    833 __ cmp(Rint, Rtmp);
>>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>>    835 #endif
>
> I still see this code is removed. Did you updated webrev?
>
>>>
>>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>>
>>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63
>
> Yes. We don't build JVM with C1 only for 64 bit.
>
>>>
>>> 4. In thread_solaris_sparc.hpp, remove line 67.
>
> Yes.
>
> Other compiler code changes seems fine.
>
> Thanks,
> Vladimir
>
>>>
>>> Thanks, Harold
>>>
>>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>>> 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
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

George Triantafillou
In reply to this post by Vladimir Kozlov
Hi Vladimir,

On 4/7/2017 1:34 PM, Vladimir Kozlov wrote:
> Hi George,
>
> I noticed that you did not changed code guarded by NOT_LP64() and
> LP64_ONLY() macros.
>
> Also the code in src/os_cpu/linux_sparc/vm is not changed.
My understanding is that this issue only covers solaris_sparc.

-George

>
> Vladimir
>
> On 4/7/17 10:26 AM, Vladimir Kozlov wrote:
>> On 4/7/17 8:04 AM, George Triantafillou wrote:
>>> Thanks Harold, I've made the suggested changes.
>>>
>>> -George
>>>
>>> On 4/7/2017 10:34 AM, harold seigel wrote:
>>>> Hi George,
>>>>
>>>> Most of the changes look good.  A few comments:
>>>>
>>>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>>>> just the "&& defined(_LP64)"
>>>>
>>>>    831 #if defined(ASSERT) && defined(_LP64)
>>>>    832 __ signx(Rint, Rtmp);
>>>>    833 __ cmp(Rint, Rtmp);
>>>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>>>    835 #endif
>>
>> I still see this code is removed. Did you updated webrev?
>>
>>>>
>>>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>>>
>>>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63
>>
>> Yes. We don't build JVM with C1 only for 64 bit.
>>
>>>>
>>>> 4. In thread_solaris_sparc.hpp, remove line 67.
>>
>> Yes.
>>
>> Other compiler code changes seems fine.
>>
>> Thanks,
>> Vladimir
>>
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>>>> 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
>>>>>
>>>>
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Vladimir Kozlov
We will never support 32-bit VM in future JDK releases on all SPARC
platforms which includes Linux (we only build 64 bit JDK on it)! So you
need to clean it too.

Vladimir

On 4/7/17 12:21 PM, George Triantafillou wrote:

> Hi Vladimir,
>
> On 4/7/2017 1:34 PM, Vladimir Kozlov wrote:
>> Hi George,
>>
>> I noticed that you did not changed code guarded by NOT_LP64() and
>> LP64_ONLY() macros.
>>
>> Also the code in src/os_cpu/linux_sparc/vm is not changed.
> My understanding is that this issue only covers solaris_sparc.
>
> -George
>>
>> Vladimir
>>
>> On 4/7/17 10:26 AM, Vladimir Kozlov wrote:
>>> On 4/7/17 8:04 AM, George Triantafillou wrote:
>>>> Thanks Harold, I've made the suggested changes.
>>>>
>>>> -George
>>>>
>>>> On 4/7/2017 10:34 AM, harold seigel wrote:
>>>>> Hi George,
>>>>>
>>>>> Most of the changes look good.  A few comments:
>>>>>
>>>>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>>>>> just the "&& defined(_LP64)"
>>>>>
>>>>>    831 #if defined(ASSERT) && defined(_LP64)
>>>>>    832 __ signx(Rint, Rtmp);
>>>>>    833 __ cmp(Rint, Rtmp);
>>>>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>>>>    835 #endif
>>>
>>> I still see this code is removed. Did you updated webrev?
>>>
>>>>>
>>>>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>>>>
>>>>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63
>>>
>>> Yes. We don't build JVM with C1 only for 64 bit.
>>>
>>>>>
>>>>> 4. In thread_solaris_sparc.hpp, remove line 67.
>>>
>>> Yes.
>>>
>>> Other compiler code changes seems fine.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>>>>> 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
>>>>>>
>>>>>
>>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

David Holmes
In reply to this post by George Triantafillou
Hi George,

On 8/04/2017 5:19 AM, George Triantafillou wrote:
> Hi Vladimir,
>
> Thanks for your comments.
>
> Updated webrev:
> http://cr.openjdk.java.net/~gtriantafill/8150388-webrev/webrev.01/
> <http://cr.openjdk.java.net/%7Egtriantafill/8150388-webrev/webrev.01/>

There are also changes needed in shared code:

./vm/utilities/globalDefinitions_sparcWorks.hpp**
./vm/utilities/globalDefinitions_gcc.hpp

** which could also do with some Solaris 8 clean out!

Thanks,
David

> -George
>
> On 4/7/2017 1:26 PM, Vladimir Kozlov wrote:
>> On 4/7/17 8:04 AM, George Triantafillou wrote:
>>> Thanks Harold, I've made the suggested changes.
>>>
>>> -George
>>>
>>> On 4/7/2017 10:34 AM, harold seigel wrote:
>>>> Hi George,
>>>>
>>>> Most of the changes look good.  A few comments:
>>>>
>>>> 1. In stubGenerator_sparc.cpp, lines 831-835 should not be deleted,
>>>> just the "&& defined(_LP64)"
>>>>
>>>>    831 #if defined(ASSERT) && defined(_LP64)
>>>>    832 __ signx(Rint, Rtmp);
>>>>    833 __ cmp(Rint, Rtmp);
>>>>    834 __ breakpoint_trap(Assembler::notEqual, Assembler::xcc);
>>>>    835 #endif
>>
>> I still see this code is removed. Did you updated webrev?
>>
>>>>
>>>> 2. In atomic_solaris_sparc.hpp: remove lines 245 and 372
>>>>
>>>> 3. In prefetch_solaris_sparc.inline.hpp, remove lines 30 and 63
>>
>> Yes. We don't build JVM with C1 only for 64 bit.
>>
>>>>
>>>> 4. In thread_solaris_sparc.hpp, remove line 67.
>>
>> Yes.
>>
>> Other compiler code changes seems fine.
>>
>> Thanks,
>> Vladimir
>>
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 4/7/2017 9:43 AM, George Triantafillou wrote:
>>>>> 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
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Kim Barrett
In reply to this post by George Triantafillou
> On Apr 7, 2017, at 10:47 AM, George Triantafillou <[hidden email]> 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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------  
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
src/cpu/sparc/vm/stubGenerator_sparc.cpp
Removed block starting with:
 831 #if defined(ASSERT) && defined(_LP64)

That looks like an incorrect removal.

------------------------------------------------------------------------------
src/cpu/sparc/vm/stubGenerator_sparc.cpp
1232     if (!aligned)

1233     {

Please move the { to line 1232.

------------------------------------------------------------------------------
src/cpu/sparc/vm/stubGenerator_sparc.cpp
1334     } else

1335     {

Please move the { to line 1334.

------------------------------------------------------------------------------
src/cpu/sparc/vm/stubGenerator_sparc.cpp
1445     if (!aligned)

1446     {

Please move the { to line 1445.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------  
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Kim Barrett
> On Apr 15, 2017, at 4:27 AM, Kim Barrett <[hidden email]> wrote:
> 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.

And if we did, the resulting value seems suspect. But someone with
more knowledge than I have should figure out what should really be
done here. It's not the kind of mostly mechanical change that this RFE
is about.

> 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.

That was wrong of me. As written, the #if/#endif can be just removed.
It would be a change to remove just the _LP64 part.

OTOH, the comment at the head of the protected section suggests this
is mostly for sparc-v8 support, and I believe we don't support that
anymore, so further cleanup perhaps to be had here, in some different
RFE.  (Assuming I'm remembering correctly that we no longer support
v8, is there an existing RFE for that cleanup?)

> 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.

As with the one above, this comment is similarly wrong.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

George Triantafillou
In reply to this post by Kim Barrett
Hi Kim,

Comments inline:

On 4/15/2017 4:27 AM, Kim Barrett wrote:

>> On Apr 7, 2017, at 10:47 AM, George Triantafillou <[hidden email]> 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.

>
> ------------------------------------------------------------------------------
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8150388: Remove SPARC 32-bit support

Kim Barrett
On Apr 19, 2017, at 1:54 PM, George Triantafillou <[hidden email]> 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 <[hidden email]> 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.

Loading...