https://bugs.kde.org/show_bug.cgi?id=523843
--- Comment #13 from Martin Cermak <[email protected]> --- Created attachment 196312 --> https://bugs.kde.org/attachment.cgi?id=196312&action=edit proposed patch Hi Mark, thank you for the review! Let me address it bottom up: (In reply to Mark Wielaard from comment #12) [ ... stuff deleted ... ] > Just one nitpick. callHelperAndClearArgs is now always called with > zero n_arg_ws (because you restore the stack yourself). > And this is the only caller. Which means you could just delete > that whole function and just use: > > addInstr(env, X86Instr_Call( cc, (Addr)cee->addr, cee->regparms, *retloc)); > > If you wanted to clean this up. Absolutely, great catch, Updated. > > (In reply to Mark Wielaard from comment #10) > > > (In reply to Martin Cermak from comment #9) > > > > > > But you do know (the calculated) argbytes for the call. So you could save > > > the subtraction > > > and addition by checking argbytes % 16 == 0. You would still need to make > > > sure sp is > > > aligned, but then pushing the arguments will keep it aligned. > > > > > > Untested: > > > > > > /* Compute the padding: Take SP, subtract argbytes, 16-round down, > > > and add argbytes back. Get ready for true arg push */ > > > HReg tmp = newVRegI(env); > > > addInstr(env, mk_iMOVsd_RR(hregX86_ESP(), tmp)); > > > if ( argbytes % 16 != 0) > > > addInstr(env, X86Instr_Alu32R(Xalu_SUB, X86RMI_Imm(argbytes), tmp)); > > > addInstr(env, X86Instr_Alu32R(Xalu_AND, X86RMI_Imm(0xfffffff0), tmp)); > > > if ( argbytes % 16 != 0) > > > addInstr(env, X86Instr_Alu32R(Xalu_ADD, X86RMI_Imm(argbytes), tmp)); > > > addInstr(env, mk_iMOVsd_RR(tmp, hregX86_ESP())); > > > > I've verified earlier that the misalignment may come either from the > > doHelperCall() callers, or from the args being pushed onto the stack > > via pushArg(). See Comment #8 "BUT ...". > > > > Although we know the calculated arg byte size, we don't know if we > > started with an aligned address at the doHelperCall() entry. The check > > suggested above is almost equivalent to my alignment sanity check that > > I've just removed because of its performance penalty, and also because, > > as you say, it clutters the code. The only difference is that you use the > > modulo operator while I'm using a bitmask. > > The checks are different. My proposed check only tests whether the arg byte > size is zero modulo 16. If so it doesn't emit the SUB and ADD instructions > because then we know pushing the arguments will keep the stack pointer > 16 byte aligned. We still have to make sure that is true of course. > > So the proposed check would reduce the code that needs executing. > It might be it isn't really significant, but I admit I didn't matter. > And it is kind of a micro-optimization. > > I am OK with you not including it, but hopefully you do it because > you believe the optimization isn't worth it. And not because you > believe it is more expensive. > > > Based on my perf tool measurements, the alignment check is more > > expensive than the alignment code itself (subtraction, masking, > > and addition). > > Agreed, that makes sense. I just don't know why you believe the check > to see if you need to care about the arg byte size is similar to that > alignment > code. Note that the alignment code would run inside the guest on every call. > While the arg byte size check only runs once on the host while creating > the helper call. I might be missing something here terribly and apologies if I do. But... We have guest insns, IR/VEX , and host insns. The doHelperCall() operates at IR/VEX level and generates host insns. What my update does is that it assumes 16-misaligned SP at the start. Then it makes space for helper args (there may be any arg count, and summary byte size of the args may be greater than 16, and also 16-misaligned too) . So, in result these two potentially (most likely) misaligned values are summed up, and a small alignment is added. At the end, this alignment lands BEFORE the args on the stack. I agree with you that IF we have 16-aligned stack (1) AND 16-aligned args (2) then we may skip this padding dance entirely. But these are two parts of a condition where (1) may only be evaluated at guest run time, and only (2) may be aval'd at the IR/VEX level. So, this can't be implemented in practice. To make implementing this possible, we'd need to move check (2) to the guest code. That's possible. But then it's more or less equivalent to my earlier alignment check that I've dropped for performance reasons. As you say ... > We still have to make sure that is true of course. ... yes, agreed! But isn't that too expensive? Here we speak of relatively costly optimization, which, in an unlikely best case, may save/skip this super cheap alignment code (3 poor instructions (add, mask, sub). So, I can't get rid of an impression that this can't pay off :) But as mentioned, I might be missing something terribly here. Please correct me if I'm wrong. ------- Improved patch attached. Please review. -- You are receiving this mail because: You are watching all bug changes.
