8248337: sparc related code clean up after solaris removal

[Vladimir Kozlov]

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