FTL OSR exit shouldn't make X86-specific assumptions
Created attachment 224326 [details] Patch
Comment on attachment 224326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224326&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1469 > + RegisterID reg = getCachedDataTempRegisterIDAndInvalidate(); > + pushPair(reg, reg); > + move(imm, reg); > + store64(reg, stackPointerRegister); > + load64(Address(stackPointerRegister, 8), reg); Although the function name is descriptive, a comment that we use and then restore the contents of data temp register would be helpful. > Source/JavaScriptCore/ftl/FTLThunks.cpp:61 > + do { > + jit.pushToSave(GPRInfo::regT0); > + stackMisalignment += MacroAssembler::pushToSaveByteOffset(); > + numberOfRequiredPops++; > + } while (stackMisalignment % stackAlignmentBytes()); Why do we need to actually "push"? Can't we just calculate the amount we should move the stack pointer down by and do one subtract op? > Source/JavaScriptCore/ftl/FTLThunks.cpp:89 > + jit.popToRestore(GPRInfo::regT1); Do we really need to pop or can we just do an add to the stack pointer?
(In reply to comment #2) > (From update of attachment 224326 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224326&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1469 > > + RegisterID reg = getCachedDataTempRegisterIDAndInvalidate(); > > + pushPair(reg, reg); > > + move(imm, reg); > > + store64(reg, stackPointerRegister); > > + load64(Address(stackPointerRegister, 8), reg); > > Although the function name is descriptive, a comment that we use and then restore the contents of data temp register would be helpful. I guess the better thing would be not to call the getCachedDataTempRegisterIDAndInvalidate() method at all and just use *any* register directly. > > > Source/JavaScriptCore/ftl/FTLThunks.cpp:61 > > + do { > > + jit.pushToSave(GPRInfo::regT0); > > + stackMisalignment += MacroAssembler::pushToSaveByteOffset(); > > + numberOfRequiredPops++; > > + } while (stackMisalignment % stackAlignmentBytes()); > > Why do we need to actually "push"? Can't we just calculate the amount we should move the stack pointer down by and do one subtract op? This loop does the calculation as well. It's a trade-off. On x86, pushes are cheaper; on ARM, stack arithmetic is cheaper. I'm going for preserving behavior on x86 for niow. > > > Source/JavaScriptCore/ftl/FTLThunks.cpp:89 > > + jit.popToRestore(GPRInfo::regT1); > > Do we really need to pop or can we just do an add to the stack pointer? It's a trade-off. On x86, the pops are cheaper.
Created attachment 224393 [details] the patch
Comment on attachment 224393 [details] the patch r=me
Comment on attachment 224393 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=224393&action=review r=me > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1465 > + RegisterID reg = dataTempRegister; I don't have problems using getCachedDataTempRegisterIDAndInvalidate(). More grappling with how we push an immediate on ARM64. It just a little ugly that it takes a store pair, store, move multiple(s) and a load to push an immediate on ARM64 without impacting any GR. I did a quick look for other alternatives, but did find any.
Landed in http://trac.webkit.org/changeset/164228