8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base

Co-authored-by: Lutz Schmidt <lucy@openjdk.org>
Reviewed-by: mdoerr, lucy

Author: tstuefe

src/hotspot/cpu/s390/macroAssembler_s390.cpp

@@ -3603,6 +3603,7 @@ void MacroAssembler::encode_klass_not_null(Register dst, Register src) {
   Register current = (src != noreg) ? src : dst; // Klass is in dst if no src provided. (dst == src) also possible.
   address  base    = CompressedKlassPointers::base();
   int      shift   = CompressedKlassPointers::shift();
+  bool     need_zero_extend = base != 0;
   assert(UseCompressedClassPointers, "only for compressed klass ptrs");
 
   BLOCK_COMMENT("cKlass encoder {");
@@ -3619,28 +3620,76 @@ void MacroAssembler::encode_klass_not_null(Register dst, Register src) {
   bind(ok);
 #endif
 
-  if (base != NULL) {
-    unsigned int base_h = ((unsigned long)base)>>32;
-    unsigned int base_l = (unsigned int)((unsigned long)base);
-    if ((base_h != 0) && (base_l == 0) && VM_Version::has_HighWordInstr()) {
-      lgr_if_needed(dst, current);
-      z_aih(dst, -((int)base_h));     // Base has no set bits in lower half.
-    } else if ((base_h == 0) && (base_l != 0)) {
-      lgr_if_needed(dst, current);
-      z_agfi(dst, -(int)base_l);
-    } else {
-      load_const(Z_R0, base);
-      lgr_if_needed(dst, current);
-      z_sgr(dst, Z_R0);
-    }
-    current = dst;
-  }
+  // Scale down the incoming klass pointer first.
+  // We then can be sure we calculate an offset that fits into 32 bit.
+  // More generally speaking: all subsequent calculations are purely 32-bit.
   if (shift != 0) {
     assert (LogKlassAlignmentInBytes == shift, "decode alg wrong");
     z_srlg(dst, current, shift);
     current = dst;
   }
-  lgr_if_needed(dst, current); // Move may be required (if neither base nor shift != 0).
+
+  if (base != NULL) {
+    // Use scaled-down base address parts to match scaled-down klass pointer.
+    unsigned int base_h = ((unsigned long)base)>>(32+shift);
+    unsigned int base_l = (unsigned int)(((unsigned long)base)>>shift);
+
+    // General considerations:
+    //  - when calculating (current_h - base_h), all digits must cancel (become 0).
+    //    Otherwise, we would end up with a compressed klass pointer which doesn't
+    //    fit into 32-bit.
+    //  - Only bit#33 of the difference could potentially be non-zero. For that
+    //    to happen, (current_l < base_l) must hold. In this case, the subtraction
+    //    will create a borrow out of bit#32, nicely killing bit#33.
+    //  - With the above, we only need to consider current_l and base_l to
+    //    calculate the result.
+    //  - Both values are treated as unsigned. The unsigned subtraction is
+    //    replaced by adding (unsigned) the 2's complement of the subtrahend.
+
+    if (base_l == 0) {
+      //  - By theory, the calculation to be performed here (current_h - base_h) MUST
+      //    cancel all high-word bits. Otherwise, we would end up with an offset
+      //    (i.e. compressed klass pointer) that does not fit into 32 bit.
+      //  - current_l remains unchanged.
+      //  - Therefore, we can replace all calculation with just a
+      //    zero-extending load 32 to 64 bit.
+      //  - Even that can be replaced with a conditional load if dst != current.
+      //    (this is a local view. The shift step may have requested zero-extension).
+    } else {
+      if ((base_h == 0) && is_uimm(base_l, 31)) {
+        // If we happen to find that (base_h == 0), and that base_l is within the range
+        // which can be represented by a signed int, then we can use 64bit signed add with
+        // (-base_l) as 32bit signed immediate operand. The add will take care of the
+        // upper 32 bits of the result, saving us the need of an extra zero extension.
+        // For base_l to be in the required range, it must not have the most significant
+        // bit (aka sign bit) set.
+        lgr_if_needed(dst, current); // no zero/sign extension in this case!
+        z_agfi(dst, -(int)base_l);   // base_l must be passed as signed.
+        need_zero_extend = false;
+        current = dst;
+      } else {
+        // To begin with, we may need to copy and/or zero-extend the register operand.
+        // We have to calculate (current_l - base_l). Because there is no unsigend
+        // subtract instruction with immediate operand, we add the 2's complement of base_l.
+        if (need_zero_extend) {
+          z_llgfr(dst, current);
+          need_zero_extend = false;
+        } else {
+          llgfr_if_needed(dst, current);
+        }
+        current = dst;
+        z_alfi(dst, -base_l);
+      }
+    }
+  }
+
+  if (need_zero_extend) {
+    // We must zero-extend the calculated result. It may have some leftover bits in
+    // the hi-word because we only did optimized calculations.
+    z_llgfr(dst, current);
+  } else {
+    llgfr_if_needed(dst, current); // zero-extension while copying comes at no extra cost.
+  }
 
   BLOCK_COMMENT("} cKlass encoder");
 }