Bug 128890

Summary: FTL OSR exit shouldn't make X86-specific assumptions
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: New BugsAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 112840    
Attachments:
Description Flags
Patch
none
the patch mhahnenberg: review+

Filip Pizlo
Reported 2014-02-16 18:09:04 PST
FTL OSR exit shouldn't make X86-specific assumptions
Attachments
Patch (7.29 KB, patch)
2014-02-16 18:10 PST, Filip Pizlo
no flags
the patch (7.25 KB, patch)
2014-02-17 10:26 PST, Filip Pizlo
mhahnenberg: review+
Filip Pizlo
Comment 1 2014-02-16 18:10:27 PST
Michael Saboff
Comment 2 2014-02-17 09:18:11 PST
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?
Filip Pizlo
Comment 3 2014-02-17 10:21:28 PST
(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.
Filip Pizlo
Comment 4 2014-02-17 10:26:24 PST
Created attachment 224393 [details] the patch
Mark Hahnenberg
Comment 5 2014-02-17 10:35:20 PST
Comment on attachment 224393 [details] the patch r=me
Michael Saboff
Comment 6 2014-02-17 10:36:26 PST
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.
Filip Pizlo
Comment 7 2014-02-17 10:54:16 PST
Note You need to log in before you can comment on or make changes to this bug.