https://bugs.kde.org/show_bug.cgi?id=523843
--- Comment #12 from Mark Wielaard <[email protected]> --- (In reply to Martin Cermak from comment #11) > Created attachment 196256 [details] > proposed patch > > (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. > [ ... stuff deleted ... ] > > > I think sse4-x86 is kind of worst case because it uses a lot of helper > > calls. > > But this doesn't look so bad. And this case (3) seems best. So lets go with > > this one. > > Agreed, I've cleaned the code up a little bit and am attaching it as a > proposed > patch. It tests fine locally for me. Does it look good from your > perspective? > Please, review. Yes it looks good. 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. -- You are receiving this mail because: You are watching all bug changes.
