8248337: sparc related code clean up after solaris removal

[David Holmes]

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.

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

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.

---

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
>