Bug 227039 – Use ldp and stp more for saving / restoring registers on ARM64.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Details coming in ChangeLog.

[Radar WebKit Bug Importer]

<rdar://problem/79354736>

[Mark Lam]

Created attachment 431463 [details]
proposed patch.

[Mark Lam]

Created attachment 431464 [details]
proposed patch.

[Mark Lam]

Created attachment 431472 [details]
patch to test workaround for gcc weakness.

[Mark Lam]

Created attachment 431473 [details]
take 2 at working around gcc being weak sauce.

[Mark Lam]

Created attachment 431479 [details]
take 3: testing workaround for gcc being weak sauce.

[Mark Lam]

Created attachment 431482 [details]
proposed patch.

[Mark Lam]

Created attachment 431483 [details]
proposed patch.

[Mark Lam]

Created attachment 431484 [details]
proposed patch.

[Saam Barati]

Comment on attachment 431484 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=431484&action=review

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1232
> +void AssemblyHelpers::emitSaveCalleeSavesFor(const RegisterAtOffsetList* calleeSaves)
> +{
> +    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
> +    unsigned registerCount = calleeSaves->size();
> +
> +    StoreRegSpooler spooler(*this, framePointerRegister);
> +
> +    unsigned i = 0;
> +    for (; i < registerCount; i++) {
> +        RegisterAtOffset entry = calleeSaves->at(i);
> +        if (dontSaveRegisters.contains(entry.reg()))
> +            continue;
> +        spooler.storeGPR(entry);
> +    }
> +    spooler.flushGPR();
> +    for (; i < registerCount; i++) {
> +        RegisterAtOffset entry = calleeSaves->at(i);
> +        if (dontSaveRegisters.contains(entry.reg()))
> +            continue;
> +        spooler.storeFPR(entry);
> +    }
> +    spooler.flushFPR();
> +}

This code doesn't really make sense given you ignore allFPRs. I think it needs reconsidering. Either handle FPRs properly, or assert we don't see them.

For example, the second loop is completely dead since we iterate everything in the first loop.

The second loop is also dead because we ignore FPRs above.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1256
> +void AssemblyHelpers::emitRestoreCalleeSavesFor(const RegisterAtOffsetList* calleeSaves)
> +{
> +    RegisterSet dontRestoreRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
> +    unsigned registerCount = calleeSaves->size();
> +    
> +    LoadRegSpooler spooler(*this, framePointerRegister);
> +
> +    unsigned i = 0;
> +    for (; i < registerCount; i++) {
> +        RegisterAtOffset entry = calleeSaves->at(i);
> +        if (dontRestoreRegisters.get(entry.reg()))
> +            continue;
> +        spooler.loadGPR(entry);
> +    }
> +    spooler.flushGPR();
> +    for (; i < registerCount; i++) {
> +        RegisterAtOffset entry = calleeSaves->at(i);
> +        if (dontRestoreRegisters.get(entry.reg()))
> +            continue;
> +        spooler.loadFPR(entry);
> +    }
> +    spooler.flushFPR();
> +}

ditto

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1286
> +        if (!entry.reg().isGPR())
> +            break;

They're guaranteed to be sorted GPRs then FPRs?

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1323
> +    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());

were you gonna rebaseline this? Let's not ignore fprs. And instead, assert in the loop below. That's less error prone in the future.

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:46
> +        ASSERT(entry.reg().isGPR());

nit: RELEASE_ASSERT

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:69
> +    void flushGPR()

name nit: I sorta feel like this should be called "commit" or something like that?

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:79
> +    void handleFPR(const RegisterAtOffset& entry)

the code for FPR and GPR are so similar. Can we share code somehow?

same in the flush functions

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:81
> +        ASSERT(entry.reg().isFPR());

nit: RELEASE_ASSERT

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:342
> +    };

no semicolon

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:398
> +                if constexpr (regTypeIsGPR) {

shouldn't this be like an assert?

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:407
> +                if constexpr (regTypeIsGPR) {

ditto

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:418
> +                if constexpr (regTypeIsGPR) {

ditto

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:426
> +                if constexpr (regTypeIsGPR) {

ditto

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:461
> +    };

no semicolon

[Mark Lam]

Comment on attachment 431484 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=431484&action=review

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1232
>> +}
> 
> This code doesn't really make sense given you ignore allFPRs. I think it needs reconsidering. Either handle FPRs properly, or assert we don't see them.
> 
> For example, the second loop is completely dead since we iterate everything in the first loop.
> 
> The second loop is also dead because we ignore FPRs above.

After Tadeu's fix, we will need this loop to handle FPRs as well.  Leaving as is.

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1256
>> +}
> 
> ditto

Ditto.

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1286
>> +            break;
> 
> They're guaranteed to be sorted GPRs then FPRs?

Yes.  RegisterAtOffsetList can only be constructed from a RegisterSet, by iterating the RegisterSet from begin() to end().   RegisterSet organizes its tracked registers with a bitmap, where bits 0 to MacroAssembler::numGPRs are for GPRReg, and bits MacroAssembler::numGPRs to MacroAssembler::numGPRs + MacroAssembler::numFPRs are for FPRRegs.  As a result, RegisterAtOffsetList is sorted with GPRRegs first (from low to high) followed by FPRRegs (from low to high).

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1323
>> +    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
> 
> were you gonna rebaseline this? Let's not ignore fprs. And instead, assert in the loop below. That's less error prone in the future.

CodeBlock here is always a baseline codeBlock.  Hence, we're only visiting RegisterAtOffsetList::llintBaselineCalleeSaveRegisters(), which is guaranteed to not have any FPRs.  The rebased code will have a `RELEASE_ASSERT(entry.reg().isGPR())`.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:46
>> +        ASSERT(entry.reg().isGPR());
> 
> nit: RELEASE_ASSERT

Changed.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:69
>> +    void flushGPR()
> 
> name nit: I sorta feel like this should be called "commit" or something like that?

Renamed to finalizeGPR.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:79
>> +    void handleFPR(const RegisterAtOffset& entry)
> 
> the code for FPR and GPR are so similar. Can we share code somehow?
> 
> same in the flush functions

Refactored into a common template.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:81
>> +        ASSERT(entry.reg().isFPR());
> 
> nit: RELEASE_ASSERT

Changed.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:342
>> +    };
> 
> no semicolon

Fixed.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:398
>> +                if constexpr (regTypeIsGPR) {
> 
> shouldn't this be like an assert?

I'll add a RELEASE_ASSERT_NOT_REACHED for the FPR case.

>> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:461
>> +    };
> 
> no semicolon

Fixed.

[Mark Lam]

Created attachment 432068 [details]
proposed patch.

[Saam Barati]

Comment on attachment 432068 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r279256: <http://trac.webkit.org/r279256>.