[Valgrind-developers] [valgrind] Bug 417452 - s390x: Force 12-bit amode for vector stores in isel

[Andreas A. <ar...@so...>]

https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=f27fe920cd321ca3cf4bc03a72879fd18bf2736f

commit f27fe920cd321ca3cf4bc03a72879fd18bf2736f
Author: Andreas Arnez <ar...@li...>
Date:   Wed Feb 12 14:13:55 2020 +0100

    Bug 417452 - s390x: Force 12-bit amode for vector stores in isel
    
    It was seen that the s390 instruction selector chose a wrong addressing
    mode for storing a vector register.  The VST instruction only handles
    short (12-bit unsigned) displacements, but a long (20-bit signed)
    displacement was generated instead, resulting in a panic:
    
    vex: the `impossible' happened:
       s390_insn_store_emit: unknown dst->tag for HRcVec128
    
    The fix prevents long displacements for vector store operations.  It also
    optimizes vector store operations from an Iex_Get, by converting them to a
    memory copy.  This optimization was already performed for integer
    registers.

Diff:
---
 NEWS                      |  1 +
 VEX/priv/host_s390_defs.c |  2 +-
 VEX/priv/host_s390_isel.c | 67 +++++++++++++++++++++++++++++++++--------------
 3 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/NEWS b/NEWS
index 79b3d7c..99ece46 100644
--- a/NEWS
+++ b/NEWS
@@ -116,6 +116,7 @@ n-i-bz  Add support for the Linux io_uring system calls
 n-i-bz  sys_statx: don't complain if both |filename| and |buf| are NULL.
 n-i-bz  Fix non-glibc build of test suite with s390x_features
 416667  gcc10 ppc64le impossible constraint in 'asm' in test_isa.
+417452  s390_insn_store_emit: dst->tag for HRcVec128
 
 
 Release 3.15.0 (12 April 2019)
diff --git a/VEX/priv/host_s390_defs.c b/VEX/priv/host_s390_defs.c
index 9ad7240..47928cb 100644
--- a/VEX/priv/host_s390_defs.c
+++ b/VEX/priv/host_s390_defs.c
@@ -6314,7 +6314,7 @@ s390_insn_memcpy(UChar size, s390_amode *dst, s390_amode *src)
    insn->variant.memcpy.src = src;
    insn->variant.memcpy.dst = dst;
 
-   vassert(size == 1 || size == 2 || size == 4 || size == 8);
+   vassert(size == 1 || size == 2 || size == 4 || size == 8 || size == 16);
 
    return insn;
 }
diff --git a/VEX/priv/host_s390_isel.c b/VEX/priv/host_s390_isel.c
index 97614c8..fff81fe 100644
--- a/VEX/priv/host_s390_isel.c
+++ b/VEX/priv/host_s390_isel.c
@@ -302,12 +302,14 @@ ulong_fits_signed_8bit(ULong val)
    return val == v;
 }
 
-/* EXPR is an expression that is used as an address. Return an s390_amode
-   for it. If select_b12_b20_only is true the returned amode must be either
-   S390_AMODE_B12 or S390_AMODE_B20. */
+/* EXPR is an expression that is used as an address. Return an s390_amode for
+   it. If no_index is true the returned amode must be either S390_AMODE_B12 or
+   S390_AMODE_B20. If short_displacement is true it must be either
+   S390_AMODE_B12 or S390_AMODE_BX12. */
 static s390_amode *
 s390_isel_amode_wrk(ISelEnv *env, IRExpr *expr,
-                    Bool select_b12_b20_only __attribute__((unused)))
+                    Bool no_index __attribute__((unused)),
+                    Bool short_displacement)
 {
    if (expr->tag == Iex_Binop && expr->Iex.Binop.op == Iop_Add64) {
       IRExpr *arg1 = expr->Iex.Binop.arg1;
@@ -328,7 +330,7 @@ s390_isel_amode_wrk(ISelEnv *env, IRExpr *expr,
          if (ulong_fits_unsigned_12bit(value)) {
             return s390_amode_b12((Int)value, s390_isel_int_expr(env, arg1));
          }
-         if (ulong_fits_signed_20bit(value)) {
+         if (!short_displacement && ulong_fits_signed_20bit(value)) {
             return s390_amode_b20((Int)value, s390_isel_int_expr(env, arg1));
          }
       }
@@ -348,7 +350,25 @@ s390_isel_amode(ISelEnv *env, IRExpr *expr)
    /* Address computation should yield a 64-bit value */
    vassert(typeOfIRExpr(env->type_env, expr) == Ity_I64);
 
-   am = s390_isel_amode_wrk(env, expr, /* B12, B20 only */ False);
+   am = s390_isel_amode_wrk(env, expr, False, False);
+
+   /* Check post-condition */
+   vassert(s390_amode_is_sane(am));
+
+   return am;
+}
+
+/* Sometimes we need an amode with short (12-bit) displacement. An example is
+   the vector-store opcode. */
+static s390_amode *
+s390_isel_amode_short(ISelEnv *env, IRExpr *expr)
+{
+   s390_amode *am;
+
+   /* Address computation should yield a 64-bit value */
+   vassert(typeOfIRExpr(env->type_env, expr) == Ity_I64);
+
+   am = s390_isel_amode_wrk(env, expr, False, True);
 
    /* Check post-condition */
    vassert(s390_amode_is_sane(am));
@@ -379,7 +399,7 @@ s390_isel_amode_b12_b20(ISelEnv *env, IRExpr *expr)
    /* Address computation should yield a 64-bit value */
    vassert(typeOfIRExpr(env->type_env, expr) == Ity_I64);
 
-   am = s390_isel_amode_wrk(env, expr, /* B12, B20 only */ True);
+   am = s390_isel_amode_wrk(env, expr, True, False);
 
    /* Check post-condition */
    vassert(s390_amode_is_sane(am) &&
@@ -4727,7 +4747,26 @@ s390_isel_stmt(ISelEnv *env, IRStmt *stmt)
 
       if (stmt->Ist.Store.end != Iend_BE) goto stmt_fail;
 
-      am = s390_isel_amode(env, stmt->Ist.Store.addr);
+      if (tyd == Ity_V128) {
+         am = s390_isel_amode_short(env, stmt->Ist.Store.addr);
+      } else {
+         am = s390_isel_amode(env, stmt->Ist.Store.addr);
+      }
+
+      /* Check whether we can use a memcpy. Currently, the restriction
+         is that both amodes need to be B12, so MVC can be emitted.
+         We do not consider a store whose data expression is a load because
+         we don't want to deal with overlapping locations. */
+      /* store(get) never overlaps*/
+      if (am->tag == S390_AMODE_B12 &&
+          stmt->Ist.Store.data->tag == Iex_Get) {
+         UInt offset = stmt->Ist.Store.data->Iex.Get.offset;
+         s390_amode *from = s390_amode_for_guest_state(offset);
+         if (from->tag == S390_AMODE_B12) {
+            addInstr(env, s390_insn_memcpy(sizeofIRType(tyd), am, from));
+            return;
+         }
+      }
 
       switch (tyd) {
       case Ity_I8:
@@ -4742,18 +4781,6 @@ s390_isel_stmt(ISelEnv *env, IRStmt *stmt)
             addInstr(env, s390_insn_mimm(sizeofIRType(tyd), am, value));
             return;
          }
-         /* Check whether we can use a memcpy here. Currently, the restriction
-            is that both amodes need to be B12, so MVC can be emitted.
-            We do not consider a store whose data expression is a load because
-            we don't want to deal with overlapping locations. */
-         /* store(get) never overlaps*/
-         if (am->tag == S390_AMODE_B12 &&
-             stmt->Ist.Store.data->tag == Iex_Get) {
-            UInt offset = stmt->Ist.Store.data->Iex.Get.offset;
-            s390_amode *from = s390_amode_for_guest_state(offset);
-            addInstr(env, s390_insn_memcpy(sizeofIRType(tyd), am, from));
-            return;
-         }
          /* General case: compile data into a register */
          src = s390_isel_int_expr(env, stmt->Ist.Store.data);
          break;