8344381: [s390x] Test failures with error: Register type is not known

[offamitkumar]

Adds SaveLiveRegister portion for vector registers also.

Depends on #22190. Once that PR gets integrated, will rebase and mark it ready for review.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8344381: [s390x] Test failures with error: Register type is not known (Bug - P3)

Reviewers

• Lutz Schmidt (@RealLucy - Reviewer) 🔄 Re-review required (review applies to c6928c4b)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22197/head:pull/22197
$ git checkout pull/22197

Update a local copy of the PR:
$ git checkout pull/22197
$ git pull https://git.openjdk.org/jdk.git pull/22197/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22197

View PR using the GUI difftool:
$ git pr show -t 22197

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22197.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back amitkumar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@offamitkumar This change is no longer ready for integration - check the PR body for details.

[openjdk]

@offamitkumar The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (c6928c4b)
• 00: Full (6645f7b2)

[offamitkumar]

I got only test failure : java/util/Spliterator/SpliteratorTraversingAndSplittingTest.java which seems to generic issue JDK-8344253

[offamitkumar]

@RealLucy can you take a look at this one. I guess this also can be treated as trivial. It will good if you can see if the register preserved for float & general register are also okay.

[TheRealMDoerr]

Looks basically good. I think that the instructions can encode the maximum frame size.

[offamitkumar]

I think that the instructions can encode the maximum frame size.

both of the instruction support displacement up to 12bit unsigned integer, which should give us flexibility of around 4000byte So I guess we should be fine.

[RealLucy]

I think that the instructions can encode the maximum frame size.

both of the instruction support displacement up to 12bit unsigned integer, which should give us flexibility of around 4000byte So I guess we should be fine.

Instead of speculation:

• GPRs: 16 registers, 8 bytes each -> 128 bytes
• FPRs: 16 registers, 8 bytes each -> 128 bytes
• VRs: 32 registers, 16 bytes each -> 512 bytes
• In total: 768 bytes.

This is an easy fit into uimm12.

[RealLucy]

Looks good.

[TheRealMDoerr]

Thanks for the update. Now, I only wonder about the >= Z_V2 condition. How does this fit to

jdk/src/hotspot/cpu/s390/s390.ad

Line 745 in dfddbca

// Attention: Only these ones are saved & restored at safepoint by RegisterSaver.

?

[offamitkumar]

Thanks for the update. Now, I only wonder about the >= Z_V2 condition. How does this fit to

jdk/src/hotspot/cpu/s390/s390.ad

Line 745 in dfddbca

// Attention: Only these ones are saved & restored at safepoint by RegisterSaver.

?

I didn't get the question actually. You mean that why didn't preserve Z_VR0 and Z_VR1 ? Or why we are preserving vector register VR16 to VR31 ?

[TheRealMDoerr]

Thanks for the update. Now, I only wonder about the >= Z_V2 condition. How does this fit to

jdk/src/hotspot/cpu/s390/s390.ad

Line 745 in dfddbca

// Attention: Only these ones are saved & restored at safepoint by RegisterSaver.

?

I didn't get the question actually. You mean that why didn't preserve Z_VR0 and Z_VR1 ? Or why we are preserving vector register VR16 to VR31 ?

What's the reason for choosing Z_V2? Why are Z_VR0 and Z_VR1 excluded? Do Z_VR0 to Z_VR15 match the FP registers? If so, why are you saving and restoring Z_V2 to Z_V15 twice?

[offamitkumar]

Do Z_VR0 to Z_VR15 match the FP registers?

Yes they do override.

If so, why are you saving and restoring Z_V2 to Z_V15 twice?

Right, so register allocator will not use them, as it is not allowed to, So no need to preserve the 2nd-half. Right. Even if we use them manually we can preserve them ourselves.

[TheRealMDoerr]

Z_V16 to Z_V31 should be preserved. Also see PPC64 implementation.

[offamitkumar]

I looked ppc-abi and your changes. I want to push some additional changes:

diff --git a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
index 57d38170a4d..3fff500e47c 100644
--- a/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
+++ b/src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp
@@ -240,6 +240,7 @@ int SaveLiveRegisters::iterate_over_register_mask(IterationAction action, int of
   int reg_save_index = 0;
   RegMaskIterator live_regs_iterator(_reg_mask);
 
+  // Going to preserve the volatile registers which can be used by Register Allocator.
   while(live_regs_iterator.has_next()) {
     const OptoReg::Name opto_reg = live_regs_iterator.next();
 
@@ -251,8 +252,10 @@ int SaveLiveRegisters::iterate_over_register_mask(IterationAction action, int of
     const VMReg vm_reg = OptoReg::as_VMReg(opto_reg);
     if (vm_reg->is_Register()) {
       Register std_reg = vm_reg->as_Register();
-
-      if (std_reg->encoding() >= Z_R2->encoding() && std_reg->encoding() <= Z_R15->encoding()) {
+      // Z_R0 and Z_R1 will not be allocated by the register allocator, see s390.ad (Integer Register Classes)
+      // Z_R6 to Z_R15 are saved registers, except Z_R14 (see Z-Abi)
+      if (std_reg->encoding() == Z_R14->encoding()
+          || (std_reg->encoding() >= Z_R2->encoding() && std_reg->encoding() <= Z_R5->encoding())) {
         reg_save_index++;
 
         if (action == ACTION_SAVE) {
@@ -265,7 +268,8 @@ int SaveLiveRegisters::iterate_over_register_mask(IterationAction action, int of
       }
     } else if (vm_reg->is_FloatRegister()) {
       FloatRegister fp_reg = vm_reg->as_FloatRegister();
-      if (fp_reg->encoding() >= Z_F0->encoding() && fp_reg->encoding() <= Z_F15->encoding()
+      // Z_R1 will not be allocated by the register allocator, see s390.ad (Float Register Classes)
+      if (fp_reg->encoding() >= Z_F0->encoding() && fp_reg->encoding() <= Z_F7->encoding()
           && fp_reg->encoding() != Z_F1->encoding()) {
         reg_save_index++;
 
@@ -279,7 +283,8 @@ int SaveLiveRegisters::iterate_over_register_mask(IterationAction action, int of
       }
     } else if (vm_reg->is_VectorRegister()) {
       VectorRegister vs_reg = vm_reg->as_VectorRegister();
-      if (vs_reg->encoding() >= Z_V2->encoding() && vs_reg->encoding() <= Z_V31->encoding()) {
+      // Z_V0 to Z_V15 will not be allocated by the register allocator, see s390.ad (reg class z_v_reg)
+      if (vs_reg->encoding() >= Z_V16->encoding() && vs_reg->encoding() <= Z_V31->encoding()) {
         reg_save_index += 2;
         if (action == ACTION_SAVE) {
           __ z_vst(vs_reg, Address(Z_SP, offset - reg_save_index * BytesPerWord));

Does these seem any good ? I guess I have preserved many already-saved-registers. Maybe we can get rid of that ?

[TheRealMDoerr]

This makes sense. Only volatile registers which are allocated need to be saved and restored. You could also use is_volatile() functions from the Register class.
The registers which are not allocated should not show up here because we get them from live_regs_iterator.next(). So, I think there's no need to filter them out. But you can filter them out. I have no objects. I had basically asked because of readability reasons. Sorry if my question was a bit confusing.

[offamitkumar]

This makes sense. Only volatile registers which are allocated need to be saved and restored. You could also use is_volatile() functions from the Register class. The registers which are not allocated should not show up here because we get them from live_regs_iterator.next(). So, I think there's no need to filter them out. But you can filter them out. I have no objects. I had basically asked because of readability reasons. Sorry if my question was a bit confusing.

No I guess your question was perfectly correct. My concept for "which register needs to be preserved" was a bit on the loose. But now I got idea. So thanks for that.