8248337: sparc related code clean up after solaris removal

[Yumin Qi]

HI, David

   Thanks for re-review!


Yumin

On 9/1/20 9:07 PM, David Holmes wrote:
> Hi Yumin,
>
> Update looks good. Thanks for filing the other RFEs.
>
> David
>
> On 2/09/2020 8:40 am, Yumin Qi wrote:
>> HI, Vladimir and David
>>
>>     I have updated new webrev at: http://cr.openjdk.java.net/~minqi/2020/8248337/webrev-02/
>>
>>     Filed two issues to address your concern separately:
>>
>>     1) 8252681: Retire flag UseRDPCForConstantTableBase after solaris removal
>>
>> https://bugs.openjdk.java.net/browse/JDK-8252681
>>
>>    2) 8252682: investigate PhaseChaitin::post_allocate_copy_removal after solaris removal
>>
>> https://bugs.openjdk.java.net/browse/JDK-8252682
>>
>>
>>    So following three files leave no change:
>>
>> share/opto/c2_globals.hpp
>>
>> share/opto/machnode.hpp
>>
>> share/opto/postaloc.cpp
>>
>>    Also update some copyright year for several files.
>>
>>
>> Thanks
>>
>> Yumin
>>
>>
>> On 9/1/20 10:35 AM, Vladimir Kozlov wrote:
>>> On 8/31/20 6:18 PM, David Holmes wrote:
>>>> Hi Yumin,
>>>>
>>>> On 1/09/2020 7:32 am, Yumin Qi wrote:
>>>>> HI,
>>>>>
>>>>>    Please review for
>>>>>
>>>>>    bug: https://bugs.openjdk.java.net/browse/JDK-8248337
>>>>>
>>>>> webrev:http://cr.openjdk.java.net/~minqi/2020/8248337/webrev-01/
>>>>>
>>>>>
>>>>>    Summary: After Solaris supported files removed from repo, there are some remnants which needs cleaning up. Some comments are not correct, and some refer to wrong files.
>>>>
>>>> Those changes are mostly okay but I have a few minor issues/suggestions below.
>>>>
>>>>> There is a flag seems only useful for Sparc: UseRDPCForConstantTableBase, which got removed in this patch . 
>>>>
>>>> Despite the description of the flag it is far from clear that the use of the flag affects sparc only. It affects the pinned() function so seems somewhat platform agnostic in that sense - which is why this was not dealt with in the SPARC removal process. I think this needs closer examination by the compiler folk, with a recommendation on whether it can/should be changed or not. Regardless as this is a product flag then I think this change should be factored out and we go through the appropriate deprecate/obsolete/expire process.
>>>
>>> The flag was used to use special SPARC instruction for CPUs supporting it to load base of Constant table.
>>> It is useless for other platforms. MachConstantBaseNode::pinned() method can be removed because it inherits the method from Node::pinned() which returns 'false' too.
>>>
>>> And I agree with David that it should be done separately because it is product flag.
>>>
>>>>
>>>>> Also in postaloc.cpp, the delay slot seems is only for sparc too, but I am not sure about that. Most of the patch are in comment section.
>>>>
>>>> It refers to spill slot not delay slot. I don't see anything obviously sparc specific about that block of code.
>>>
>>> Please, leave the code as it is. As David said it is about normal spill slots for all platforms.
>>> I am not sure it is SPARC specific currently with all platforms OpenJDK supports.
>>> If you want you can file RFE to replace code with assert and ask community to run a lot of testing to see if we hit the assert.
>>>
>>>>
>>>> Specific comments:
>>>>
>>>> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
>>>>
>>>> -// 64 bits items (sparc abi) even though java would only store
>>>> +// 64 bits items even though java would only store
>>>>
>>>> Should "(sparc abi)" be replaced with "(Aarch64 abi)" as you did for other platforms?
>>>>
>>>> ---
>>>>
>>>> src/hotspot/cpu/arm/frame_arm.hpp (and other files)
>>>>
>>>>     // The interpreter and adapters will extend the frame of the caller.
>>>>     // Since oopMaps are based on the sp of the caller before extension
>>>> -  // we need to know that value. However in order to compute the address
>>>> -  // of the return address we need the real "raw" sp. Since sparc already
>>>> -  // uses sp() to mean "raw" sp and unextended_sp() to mean the caller's
>>>> -  // original sp we use that convention.
>>>> +  // we need to know that value. However in order to compute the return
>>>> +  // address we need the real "raw" sp.
>>>>
>>>> I think this is losing too much information as it no longer describes the convention. I would suggest:
>>>>
>>>>     // The interpreter and adapters will extend the frame of the caller.
>>>>     // Since oopMaps are based on the sp of the caller before extension
>>>>     // we need to know that value. However in order to compute the address
>>>> -  // of the return address we need the real "raw" sp. Since sparc already
>>>> -  // uses sp() to mean "raw" sp and unextended_sp() to mean the caller's
>>>> -  // original sp we use that convention.
>>>> +  // of the return address we need the real "raw" sp. By convention we
>>>> +  // use sp() to mean "raw" sp and unextended_sp() to mean the caller's
>>>> +  // original sp.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/cpu/ppc/jniTypes_ppc.hpp
>>>>
>>>> -  // stubGenerator_sparc.cpp) reverse the argument list constructed by
>>>> +  // stubGenerator_${CPU}.cpp) reverse the argument list constructed by
>>>>
>>>> Just replace sparc with ppc as done for other platforms.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp
>>>>
>>>> -  // This greatly simplifies the cases here compared to sparc.
>>>> +  // This greatly simplifies the cases here.
>>>>
>>>> Just delete the comment as there is nothing to compare simplicity or complexity against.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/c1/c1_LIRGenerator.cpp
>>>>
>>>> -  // In 64bit the type can be long, sparc doesn't have this assert
>>>> +  // In 64bit the type can be long
>>>>     // assert(offset.type()->tag() == intTag, "invalid type");
>>>>
>>>> compiler folk should decide what to do here but I think the comment and commented out assert can just be deleted.
>>>
>>> Yes, remove commented assert too. Originally it was platform specific code - the assert was there for 32-bit.
>>>
>>> Thanks,
>>> Vladimir K
>>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/c1/c1_Runtime1.cpp
>>>>
>>>> -  case handle_exception_nofpu_id:  // Unused on sparc
>>>> +  case handle_exception_nofpu_id:  // unused.
>>>>
>>>> the new comment is incorrect as this case is not unused. I suggest just deleting the comment.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>>
>>>>>    Tests passed tier1-4
>>>>>
>>>>>
>>>>>    Thanks
>>>>>
>>>>>    Yumin
>>>>>