C1 may generate wrong code during the phase of EdgeMoveOptimizer::optimize()

[傅杰]

Hi all,

I'm sorry in my previous email, the patch is not convenient to read.

I'm porting OpenJDK to mips.
I found that the C1 compiler may generate wrong code for mips during the phase of EdgeMoveOptimizer::optimize().

I think the reason is that EdgeMoveOptimizer::optimize_moves_at_block_begin(BlockBegin* block) doesn't consider the case of branch instructions with operands.
Some platforms, such as mips, s390 and aarch64, the branch instruction may contain register operands.
If the move instruction would change the branch instruction's operand after EdgeMoveOptimizer::optimize_moves_at_block_begin(...), we can't apply it.

I made a patch based on http://hg.openjdk.java.net/jdk/jdk/rev/538dd69b60c0.

------------------------------ patch begin ------------------------------------
diff -r 538dd69b60c0 src/hotspot/share/c1/c1_LIR.cpp
--- a/src/hotspot/share/c1/c1_LIR.cpp   Thu Jun 07 22:26:02 2018 -0400
+++ b/src/hotspot/share/c1/c1_LIR.cpp   Fri Jun 08 11:41:32 2018 +0800
@@ -194,7 +194,42 @@
   }
 }

+bool LIR_OprDesc::has_common_register(LIR_Opr opr) const {
+#ifdef _LP64
+  return is_same_register(opr);
+#else
+  if (!(is_register() && opr->is_register())) return false;
+  if (!(kind_field() == opr->kind_field()))   return false;

+  if (is_single_cpu()) {
+    if (opr->is_single_cpu()) {
+      return as_register() == opr->as_register();
+    } else {
+      Register dst = as_register();
+      Register  lo = opr->as_register_lo();
+      Register  hi = opr->as_register_hi();
+      if (dst == lo || dst == hi) return true;
+    }
+
+  } else {
+    Register dst_lo = as_register_lo();
+    Register dst_hi = as_register_hi();
+
+    if (opr->is_single_cpu()) {
+      Register src = opr->as_register();
+      if (dst_lo == src || dst_hi == src) return true;
+    } else {
+      Register src_lo = opr->as_register_lo();
+      Register src_hi = opr->as_register_hi();
+      if (dst_lo == src_lo ||
+          dst_lo == src_hi ||
+          dst_hi == src_lo ||
+          dst_hi == src_hi) return true;
+    }
+  }
+  return false;
+#endif
+}

 void LIR_Op2::verify() const {
 #ifdef ASSERT
diff -r 538dd69b60c0 src/hotspot/share/c1/c1_LIR.hpp
--- a/src/hotspot/share/c1/c1_LIR.hpp   Thu Jun 07 22:26:02 2018 -0400
+++ b/src/hotspot/share/c1/c1_LIR.hpp   Fri Jun 08 11:41:32 2018 +0800
@@ -349,7 +349,7 @@
            opr->type_field() != unknown_type, "shouldn't see unknown_type");
     return type_field() == opr->type_field();
   }
-  bool is_same_register(LIR_Opr opr) {
+  bool is_same_register(LIR_Opr opr) const {
     return (is_register() && opr->is_register() &&
             kind_field() == opr->kind_field() &&
             (value() & no_type_mask) == (opr->value() & no_type_mask));
@@ -368,6 +368,8 @@
   bool is_float_kind() const   { return is_pointer() ? pointer()->is_float_kind() : (kind_field() == fpu_register); }
   bool is_oop() const;

+  bool has_common_register(LIR_Opr opr) const;
+
   // semantic for fpu- and xmm-registers:
   // * is_float and is_double return true for xmm_registers
   //   (so is_single_fpu and is_single_xmm are true)
diff -r 538dd69b60c0 src/hotspot/share/c1/c1_LinearScan.cpp
--- a/src/hotspot/share/c1/c1_LinearScan.cpp    Thu Jun 07 22:26:02 2018 -0400
+++ b/src/hotspot/share/c1/c1_LinearScan.cpp    Fri Jun 08 11:41:32 2018 +0800
@@ -6074,6 +6074,14 @@
       }
     }

+    // Some platforms, such as mips, s390 and aarch64, the branch instruction may contain register operands.
+    // If the move instruction would change the branch instruction's operand after the optimization, we can't apply it.
+    if (branch->as_Op2() != NULL) {
+      LIR_Op2* branch_op2 = (LIR_Op2*)branch;
+      if (op->result_opr()->has_common_register(branch_op2->in_opr1())) return;
+      if (op->result_opr()->has_common_register(branch_op2->in_opr2())) return;
+    }
+
     TRACE_LINEAR_SCAN(4, tty->print("----- found instruction that is equal in all %d successors: ", num_sux); op->print());

     // insert instruction at end of current block

------------------------------ patch end of her ------------------------------------

I think s390 and aarch64 will come across the same problem if they optimize the code generation of C1 using branch instructions with operands.
Thanks.

Best regards,
Fu Jie



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180608/9b6ae688/attachment-0001.html>