From: Florian Krohm Date: Sat, 12 May 2012 03:44:49 +0000 (+0000) Subject: Back out VEX r2326. It was not working correctly. The guard condition X-Git-Tag: svn/VALGRIND_3_8_1^2~151 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ebc4a739749f2270c5175d93a2802102cfac4535;p=thirdparty%2Fvalgrind.git Back out VEX r2326. It was not working correctly. The guard condition has to be evaluated after argument evaluation. Add clarifying comments in libvex_ir.h git-svn-id: svn://svn.valgrind.org/vex/trunk@2327 --- diff --git a/VEX/priv/host_s390_isel.c b/VEX/priv/host_s390_isel.c index 4d85a0ffb3..a84413e4b7 100644 --- a/VEX/priv/host_s390_isel.c +++ b/VEX/priv/host_s390_isel.c @@ -423,56 +423,6 @@ s390_expr_is_const_zero(IRExpr *expr) } -/* If EXPR can be evaluated with a single s390_insn, do so and assign the - value to register DST. If not, return NULL. */ -static s390_insn * -s390_isel_single_insn_for_int_expr(ISelEnv *env, HReg dst, IRExpr *expr) -{ - switch (expr->tag) { - case Iex_Const: { - ULong value; - const IRConst *con = expr->Iex.Const.con; - - /* Bitwise copy of the value. No sign/zero-extension */ - switch (con->tag) { - case Ico_U64: value = con->Ico.U64; break; - case Ico_U32: value = con->Ico.U32; break; - case Ico_U16: value = con->Ico.U16; break; - case Ico_U8: value = con->Ico.U8; break; - default: vpanic("s390_isel_single_insn_for_int_expr: invalid constant"); - } - - return s390_insn_load_immediate(8, dst, value); - } - - case Iex_RdTmp: { - HReg src = lookupIRTemp(env, expr->Iex.RdTmp.tmp); - return s390_insn_move(8, dst, src); - } - - case Iex_Get: { - UInt size = sizeofIRType(typeOfIRExpr(env->type_env, expr)); - s390_amode *am = s390_amode_for_guest_state(expr->Iex.Get.offset); - - return s390_insn_load(size, dst, am); - } - - case Iex_Load: { - UInt size = sizeofIRType(typeOfIRExpr(env->type_env, expr)); - s390_amode *am = s390_isel_amode(env, expr->Iex.Load.addr); - - vassert(expr->Iex.Load.end == Iend_BE); - - return s390_insn_load(size, dst, am); - } - - default: - /* too complex */ - return NULL; - } -} - - /* Call a helper (clean or dirty) Arguments must satisfy the following conditions: @@ -481,14 +431,29 @@ s390_isel_single_insn_for_int_expr(ISelEnv *env, HReg dst, IRExpr *expr) guard is a Ity_Bit expression indicating whether or not the call happens. If guard == NULL, the call is unconditional. + + Calling the helper function proceeds as follows: + + (1) The helper arguments are evaluated and their value stored in + virtual registers. + (2) The condition code is evaluated + (3) The argument values are copied from the virtual registers to the + registers mandated by the ABI. + (4) Call the helper function. + + This is not the most efficient way as step 3 generates register-to-register + moves. But it is the least fragile way as the only hidden dependency here + is that register-to-register moves (step 3) must not clobber the condition + code. Other schemes (e.g. VEX r2326) that attempt to avoid the register- + to-register add more such dependencies. Not good. Besides, it's the job + of the register allocator to throw out those reg-to-reg moves. */ static void doHelperCall(ISelEnv *env, Bool passBBP, IRExpr *guard, IRCallee *callee, IRExpr **args) { - UInt n_args, i, argreg; + UInt n_args, i, argreg, size; ULong target; - s390_insn *insn_for_arg[S390_NUM_GPRPARMS]; HReg tmpregs[S390_NUM_GPRPARMS]; s390_cc_t cc; @@ -500,6 +465,22 @@ doHelperCall(ISelEnv *env, Bool passBBP, IRExpr *guard, vpanic("doHelperCall: too many arguments"); } + argreg = 0; + + /* If we need the guest state pointer put it in a temporary arg reg */ + if (passBBP) { + tmpregs[argreg] = newVRegI(env); + addInstr(env, s390_insn_move(sizeof(ULong), tmpregs[argreg], + s390_hreg_guest_state_pointer())); + argreg++; + } + + /* Compute the function arguments into a temporary register each */ + for (i = 0; i < n_args; i++) { + tmpregs[argreg] = s390_isel_int_expr(env, args[i]); + argreg++; + } + /* Compute the condition */ cc = S390_CC_ALWAYS; if (guard) { @@ -512,66 +493,14 @@ doHelperCall(ISelEnv *env, Bool passBBP, IRExpr *guard, } } - /* Evaluate the function arguments for the helper call. - First, as the helper function is written in C, we are free to evaluate - argumeqnts in any order we like, as the evaluation order is unspecified - by the C standard. - - Secondly, if possible, we want to evaluate the arguments such that its - value ends up in the register that is mandated by the ABI for this - argument. The idea is to avoid an additional move insn from a virtual - to a real register. Currently, the register allocator is not very good - at eliminating those. - - The complication is that we need to make sure that real registers do not - get overwritten while evaluating the arguments. Consider a helper call - with two arguments. The first argument is an integer constant and the - second argument is a complex expression. If we evaluate the 1st argument - and load the constant into r2 (as mandated by ABI) and the 2nd argument - expression contains a helper call with arguments, then evaluation of the - 2nd argument will overwrite the contents of r2. Not so good. Therefore, - we first evaluate all those arguments which are complex and assign their - value to a virtual register. Then in a second pass we evaluate those - arguments where the value can be assigned to the ABI destination register - directly. There is no danger now that those can be overwritten. This is - what happens conceptually. The implementation is slightly different. */ - - argreg = 0; - - /* If we need the guest state pointer put it into the correct register */ - if (passBBP) { - insn_for_arg[argreg] = - s390_insn_move(sizeof(ULong), - make_gpr(s390_gprno_from_arg_index(argreg)), - s390_hreg_guest_state_pointer()); - argreg++; - } - - /* Compute the function arguments. Issue insns for complex arguments. - Store their value in a temporary register each. For arguments that - can be evaluated in their ABI destination register, generate the - insn but do not issue it yet. */ - for (i = 0; i < n_args; i++) { - HReg dst = make_gpr(s390_gprno_from_arg_index(argreg)); - - insn_for_arg[argreg] = - s390_isel_single_insn_for_int_expr(env, dst, args[i]); - - if (insn_for_arg[argreg] == NULL) { - tmpregs[argreg] = s390_isel_int_expr(env, args[i]); - } - argreg++; - } - - /* Assign values to ABI registers */ + /* Move the args to the final register. It is paramount, that the + code to move the registers does not clobber the condition code ! */ for (i = 0; i < argreg; i++) { - if (insn_for_arg[i]) { - addInstr(env, insn_for_arg[i]); /* issue insn */ - } else { - /* Move value to ABI register */ - HReg finalreg = make_gpr(s390_gprno_from_arg_index(i)); - addInstr(env, s390_insn_move(8, finalreg, tmpregs[i])); - } + HReg finalreg; + + finalreg = make_gpr(s390_gprno_from_arg_index(i)); + size = sizeofIRType(Ity_I64); + addInstr(env, s390_insn_move(size, finalreg, tmpregs[i])); } target = Ptr_to_ULong(callee->addr); diff --git a/VEX/pub/libvex_ir.h b/VEX/pub/libvex_ir.h index 269ad5c666..73db9e0cde 100644 --- a/VEX/pub/libvex_ir.h +++ b/VEX/pub/libvex_ir.h @@ -1647,6 +1647,9 @@ struct _IRExpr { * it may not access guest memory, since that would hide guest memory transactions from the instrumenters + * it must not assume that arguments are being evaluated in a + particular order. The oder of evaluation is unspecified. + This is restrictive, but makes the semantics clean, and does not interfere with IR optimisation. @@ -1845,9 +1848,9 @@ extern void ppIRJumpKind ( IRJumpKind ); call does not access guest state. IMPORTANT NOTE re GUARDS: Dirty calls are strict, very strict. The - arguments are evaluated REGARDLESS of the guard value. It is - unspecified the relative order of arg evaluation and guard - evaluation. + arguments are evaluated REGARDLESS of the guard value. The order of + argument evaluation is unspecified. The guard expression is evaluated + AFTER the arguments have been evaluated. */ #define VEX_N_FXSTATE 7 /* enough for FXSAVE/FXRSTOR on x86 */