Created attachment 256709 [details] Patch using StackMap size This uses the stackmap size from LLVM to restore the stack pointer to where it should be in the patchpoint. I am interested in hearing opinions on the following two points: - We may want to adapt the default patchpoint size to accommodate the additional lea opcode. - LLVM provides a stackmap/stackrestore intrinsic that we should be able to use. I am unsure what guarantees we would have with this; in particular, I fear LLVM spilling values onto the stack before the call but after saving the stack pointer, performing the call, then trying to restore those values using the stack pointer as it is supposed to be callee-save. I did not see this happen even when testing that approach with high register pressure (LLVM ends up using bp-based offsets for the spilling), but again, I am unsure how much we can rely on this. I believe messing up with the stack pointer in the patchpoint is better because it makes it completely transparent to LLVM.

(In reply to comment #1) > Created attachment 256709 [details] > Patch using StackMap size > > This uses the stackmap size from LLVM to restore the stack pointer to where > it should be in the patchpoint. I am interested in hearing opinions on the > following two points: > > - We may want to adapt the default patchpoint size to accommodate the > additional lea opcode. > > - LLVM provides a stackmap/stackrestore intrinsic that we should be able to > use. I am unsure what guarantees we would have with this; in particular, I > fear LLVM spilling values onto the stack before the call but after saving > the stack pointer, performing the call, then trying to restore those values > using the stack pointer as it is supposed to be callee-save. I did not see > this happen even when testing that approach with high register pressure > (LLVM ends up using bp-based offsets for the spilling), but again, I am > unsure how much we can rely on this. I believe messing up with the stack > pointer in the patchpoint is better because it makes it completely > transparent to LLVM. After talking offline with fpizlo, the stackmap size approach looks like the best one.

Comment on attachment 256709 [details] Patch using StackMap size View in context: https://bugs.webkit.org/attachment.cgi?id=256709&action=review r=me with suggested name change to a function. > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) Instead of calling this "emit", how about "emitRestoreStackPointer"?

(In reply to comment #3) > Comment on attachment 256709 [details] > Patch using StackMap size > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256709&action=review > > r=me with suggested name change to a function. > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) > > Instead of calling this "emit", how about "emitRestoreStackPointer"? FTR, after discussion offline, we'll keep "emit" since this is doing more than just restoring the stack pointer.

(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 256709 [details] > > Patch using StackMap size > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=256709&action=review > > > > r=me with suggested name change to a function. > > > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > > > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) > > > > Instead of calling this "emit", how about "emitRestoreStackPointer"? > > FTR, after discussion offline, we'll keep "emit" since this is doing more > than just restoring the stack pointer. Committed r186815 <http://trac.webkit.org/changeset/186815>.