Bug 227039

Summary: Use ldp and stp more for saving / restoring registers on ARM64.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ews-watchlist, gyuyoung.kim, keith_miller, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
ews-feeder: commit-queue-
proposed patch.
ews-feeder: commit-queue-
patch to test workaround for gcc weakness.
ews-feeder: commit-queue-
take 2 at working around gcc being weak sauce.
ews-feeder: commit-queue-
take 3: testing workaround for gcc being weak sauce.
ews-feeder: commit-queue-
proposed patch.
ews-feeder: commit-queue-
proposed patch.
ews-feeder: commit-queue-
proposed patch.
none
proposed patch. saam: review+

Description Mark Lam 2021-06-15 12:07:15 PDT
Details coming in ChangeLog.
Comment 1 Radar WebKit Bug Importer 2021-06-15 12:07:43 PDT
<rdar://problem/79354736>
Comment 2 Mark Lam 2021-06-15 12:10:53 PDT
Created attachment 431463 [details]
proposed patch.
Comment 3 Mark Lam 2021-06-15 12:19:30 PDT
Created attachment 431464 [details]
proposed patch.
Comment 4 Mark Lam 2021-06-15 13:10:29 PDT
Created attachment 431472 [details]
patch to test workaround for gcc weakness.
Comment 5 Mark Lam 2021-06-15 13:29:03 PDT
Created attachment 431473 [details]
take 2 at working around gcc being weak sauce.
Comment 6 Mark Lam 2021-06-15 14:17:30 PDT
Created attachment 431479 [details]
take 3: testing workaround for gcc being weak sauce.
Comment 7 Mark Lam 2021-06-15 14:34:53 PDT
Created attachment 431482 [details]
proposed patch.
Comment 8 Mark Lam 2021-06-15 14:41:54 PDT
Created attachment 431483 [details]
proposed patch.
Comment 9 Mark Lam 2021-06-15 14:46:20 PDT
Created attachment 431484 [details]
proposed patch.
Comment 10 Saam Barati 2021-06-16 09:30:41 PDT
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
Comment 11 Mark Lam 2021-06-23 10:10:54 PDT
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.
Comment 12 Mark Lam 2021-06-23 10:35:38 PDT
Created attachment 432068 [details]
proposed patch.
Comment 13 Saam Barati 2021-06-24 16:30:21 PDT
Comment on attachment 432068 [details]
proposed patch.

r=me
Comment 14 Mark Lam 2021-06-24 17:07:59 PDT
Thanks for the review.  Landed in r279256: <http://trac.webkit.org/r279256>.