https://bugs.kde.org/show_bug.cgi?id=523843
--- Comment #10 from Mark Wielaard <[email protected]> --- (In reply to Martin Cermak from comment #9) > Created attachment 196135 [details] > proposed patch OK, so the "trick" we are using is that we look at what will be pushed onto the stack, subtract that, then make sure the stack is 16 byte aligned (downwards), add the stack argument usage again. So we know that after all arguments are pushed on the stack the stack is properly aligned. That does mean we need to save the stack and restore it before returning. You have to do this using guest instructions, since you don't know the guest stack pointer when doHelperCall is called. 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())); It might not really matter in benchmarks, but maybe we are lucky that some hot helpercall has exactly 0 or 16 bytes of arguments on the stack. /* Finally, generate the call itself. This needs the *retloc value set in the switch above, which is why it's at the end. */ - callHelperAndClearArgs( env, cc, cee, n_arg_ws, *retloc ); + callHelperAndClearArgs( env, cc, cee, 0, *retloc ); + + /* After the call, restore the saved stack pointer. */ + addInstr(env, mk_iMOVsd_RR( saved_esp, hregX86_ESP() )); I had to think about this one for a bit. You don't pass n_arg_ws because you don't want/need to add the argument space to the stack pointer anymore. Instead you just restore it directly from the new temp reg you used to save it. OK. > The attached patch computes the padding dynamically. This time the > (temporarily enabled) optional alignment check fully passes for whole the > testsuite, so imho this patch is correct. The optional alignment check > appears to be pretty performance-expensive, so it is disabled by default > (#define ALIGNMENT_SANITY 0). That is a nice check. But I think if you are going to disabled it by default you may just remove it completely. It does clutter the code a little. Just make sure that there is a reference to this bug in the code/comment so someone in the future can find this discussion (and the extra sanity checks). (But you can also leave it in, if you like.) > So, now we have 3 solutions to this problem. > 1) bug 523626 ... This is now part of the master branch. It is based on > __attribute__((force_align_arg_pointer)) set for g_calc_mpsadbw(). > 2) bug 523843 ... This is now in branch > users/mcermak/try-bug523843-stack-alignment. It is based on clang > -mstack-alignment=16 -mstackrealign being applied project wide (for -m32) > 3) bug 523843 again ... this is the attached patch. This dynamically > computes the padding as part of the doHelperCall(). > > Quick performance comparison for the none/tests/x86/sse4-x86 testcase: 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. -- You are receiving this mail because: You are watching all bug changes.
