Bug 128890 – FTL OSR exit shouldn't make X86-specific assumptions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Filip Pizlo]

FTL OSR exit shouldn't make X86-specific assumptions

[Filip Pizlo]

Created attachment 224326 [details]
Patch

[Michael Saboff]

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]

(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]

Created attachment 224393 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 224393 [details]
the patch

r=me

[Michael Saboff]

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]

Landed in http://trac.webkit.org/changeset/164228