https://bugs.kde.org/show_bug.cgi?id=523843
--- Comment #14 from Mark Wielaard <[email protected]> --- Hi Martin, (In reply to Martin Cermak from comment #13) > Created attachment 196312 [details] > proposed patch > > (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. Nice. Less code. > > > (In reply to Mark Wielaard from comment #10) > > > > (In reply to Martin Cermak from comment #9) > > 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. BTW. I am not saying your patch is wrong. It is correct. I am really just suggesting a (possible not worth it) optimization. > What my update does is that it assumes 16-misaligned SP at the start. Right. We cannot guarantee SP is 16 bytes aligned be the VEX generated code doesn't guarantee that. Also in doHelperCall (running on the host) we don't really know anything about SP, so all we can do is generate IR that inspects and adjusts SP on the guest. > 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) . Correct. The only guarantee here is that the arguments need a multiple of 4 on the guest stack. But that doesn't guarantee they take up a multiple of 16 bytes. The difference with the guest SP value however is that we do know the size needed in doHelperCall (on the host). > 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. Correct. And this then means that after that the SP is 16 bytes aligned. > 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. Right, (1) must be done in the guest, (2) may be checked in the host (and if it holds then it doesn't need generating a check/adjustment in the guest). > 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. So this is the only thing we seem to disagree on. My claim is that it is NOT equivalent to the alignment check, which does more work (precisely because it is a sanity check, so it double checks everything). Since we know on the host in doHelperCall how big argbytes is we can avoid the addition/subtraction dance in the guest if it already is a multiple of 16 bytes. In that case the only thing we need to do in the guest is adjusting SP to make sure it is 16 bytes aligned. Which is why I suggesting we do the following: 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())); So we do what you suggest, add argbytes to esp, mask to 16 bytes, subtract argbytes. But if we know argbytes is a multiple of 16 we just mask esp to 16 bytes. > 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 :) So it saves 2 of those 3 instructions in the (unlikely?) case argbytes is not a multiple of 16 bytes. That might indeed not be worth it. No argument there. > Improved patch attached. Please review. Looks good. Please take a look at the suggested optimization. But if you still believe it is bogus please feel free to push as is. -- You are receiving this mail because: You are watching all bug changes.
