From abbd80e7e23deb1f27fb50410c8b3a3f7cdfb646 Mon Sep 17 00:00:00 2001 From: Andreas Arnez Date: Wed, 12 Feb 2020 14:13:55 +0100 Subject: [PATCH] Bug 417452 - s390_insn_store_emit: dst->tag for HRcVec128 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. --- VEX/priv/host_s390_defs.c | 2 +- VEX/priv/host_s390_isel.c | 67 +++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/VEX/priv/host_s390_defs.c b/VEX/priv/host_s390_defs.c index 9ad7240c4..47928cbe1 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 97614c873..fff81fe0f 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; -- 2.23.0