8248337: sparc related code clean up after solaris removal

[Yumin Qi]

HI, Vladimir

   Thanks for review!

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.
>
I will file a bug for this be handled separately.
>>
>>> 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.
>
OK, I will keep it as it was. I will file a RFE and assign it to the right group (compiler) for further investigation.
>>
>> 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.
>
OK, will remove  them all.


Thanks

Yumin

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