8248337: sparc related code clean up after solaris removal

[David Holmes]

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