Changeset 165196 in webkit


Ignore:
Timestamp:
Mar 6, 2014, 10:33:18 AM (11 years ago)
Author:
[email protected]
Message:

Clarify how we deal with "special" registers
​https://bugs.webkit.org/show_bug.cgi?id=129806

Reviewed by Michael Saboff.

Previously we had two different places that defined what "stack" registers are, a thing
called "specialRegisters" that had unclear meaning, and a really weird "firstRealRegister"/
"secondRealRegister"/"nextRegister" idiom in MacroAssembler that appeared to only be used by
one place and had a baked-in notion of what it meant for a register to be "real" or not.

It's not cool to use words like "real" and "special" to describe registers, especially if you
fail to qualify what that means. This originally made sense on X86 - "real" registers were
the ones that weren't "stack related" (so "real" was the opposite of "stack"). But on ARM64,
you also have to worry about the LR register, which we'd want to say is "not real" but it's
also not a "stack" register. This got super confusing.

So, this patch removes any mention of "real" registers, consolidates the knowledge of what is
a "stack" register, and uses the word special only in places where it's clearly defined and
where no better word comes to mind.

This cleans up the code and fixes what seems like it was probably a harmless ARM64 bug: the
Reg and RegisterSet data structures would sometimes think that FP was Q0. Somehow this
magically didn't break anything because you never need to save/restore either FP or Q0, but
it was still super weird.

  • assembler/ARM64Assembler.h:

(JSC::ARM64Assembler::lastRegister):

  • assembler/MacroAssembler.h:

(JSC::MacroAssembler::nextRegister):

  • ftl/FTLLocation.cpp:

(JSC::FTL::Location::restoreInto):

  • ftl/FTLSaveRestore.cpp:

(JSC::FTL::saveAllRegisters):
(JSC::FTL::restoreAllRegisters):

  • ftl/FTLSlowPathCall.cpp:
  • jit/RegisterSet.cpp:

(JSC::RegisterSet::reservedHardwareRegisters):
(JSC::RegisterSet::runtimeRegisters):
(JSC::RegisterSet::specialRegisters):
(JSC::RegisterSet::calleeSaveRegisters):

  • jit/RegisterSet.h:
Location:
trunk/Source/JavaScriptCore
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r165191 r165196  
     12014-03-06  Filip Pizlo  <[email protected]>
     2
     3        Clarify how we deal with "special" registers
     4        https://bugs.webkit.org/show_bug.cgi?id=129806
     5
     6        Reviewed by Michael Saboff.
     7       
     8        Previously we had two different places that defined what "stack" registers are, a thing
     9        called "specialRegisters" that had unclear meaning, and a really weird "firstRealRegister"/
     10        "secondRealRegister"/"nextRegister" idiom in MacroAssembler that appeared to only be used by
     11        one place and had a baked-in notion of what it meant for a register to be "real" or not.
     12       
     13        It's not cool to use words like "real" and "special" to describe registers, especially if you
     14        fail to qualify what that means. This originally made sense on X86 - "real" registers were
     15        the ones that weren't "stack related" (so "real" was the opposite of "stack"). But on ARM64,
     16        you also have to worry about the LR register, which we'd want to say is "not real" but it's
     17        also not a "stack" register. This got super confusing.
     18       
     19        So, this patch removes any mention of "real" registers, consolidates the knowledge of what is
     20        a "stack" register, and uses the word special only in places where it's clearly defined and
     21        where no better word comes to mind.
     22       
     23        This cleans up the code and fixes what seems like it was probably a harmless ARM64 bug: the
     24        Reg and RegisterSet data structures would sometimes think that FP was Q0. Somehow this
     25        magically didn't break anything because you never need to save/restore either FP or Q0, but
     26        it was still super weird.
     27
     28        * assembler/ARM64Assembler.h:
     29        (JSC::ARM64Assembler::lastRegister):
     30        * assembler/MacroAssembler.h:
     31        (JSC::MacroAssembler::nextRegister):
     32        * ftl/FTLLocation.cpp:
     33        (JSC::FTL::Location::restoreInto):
     34        * ftl/FTLSaveRestore.cpp:
     35        (JSC::FTL::saveAllRegisters):
     36        (JSC::FTL::restoreAllRegisters):
     37        * ftl/FTLSlowPathCall.cpp:
     38        * jit/RegisterSet.cpp:
     39        (JSC::RegisterSet::reservedHardwareRegisters):
     40        (JSC::RegisterSet::runtimeRegisters):
     41        (JSC::RegisterSet::specialRegisters):
     42        (JSC::RegisterSet::calleeSaveRegisters):
     43        * jit/RegisterSet.h:
     44
    1452014-03-06  Filip Pizlo  <[email protected]>
    246

  • trunk/Source/JavaScriptCore/assembler/ARM64Assembler.h

    r164548 r165196  
    11/*
    2  * Copyright (C) 2012 Apple Inc. All rights reserved.
     2 * Copyright (C) 2012, 2014 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    479479   
    480480    static RegisterID firstRegister() { return ARM64Registers::x0; }
    481     static RegisterID lastRegister() { return ARM64Registers::x28; }
     481    static RegisterID lastRegister() { return ARM64Registers::sp; }
    482482   
    483483    static FPRegisterID firstFPRegister() { return ARM64Registers::q0; }

  • trunk/Source/JavaScriptCore/assembler/MacroAssembler.h

    r164764 r165196  
    11/*
    2  * Copyright (C) 2008, 2012, 2013 Apple Inc. All rights reserved.
     2 * Copyright (C) 2008, 2012, 2013, 2014 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    7070public:
    7171
    72     static bool isStackRelated(RegisterID reg)
    73     {
    74         return reg == stackPointerRegister || reg == framePointerRegister;
    75     }
    76    
    77     static RegisterID firstRealRegister()
    78     {
    79         RegisterID firstRegister = MacroAssembler::firstRegister();
    80         while (MacroAssembler::isStackRelated(firstRegister))
    81             firstRegister = static_cast<RegisterID>(firstRegister + 1);
    82         return firstRegister;
    83     }
    84    
    8572    static RegisterID nextRegister(RegisterID reg)
    8673    {
    87         RegisterID result = static_cast<RegisterID>(reg + 1);
    88         while (MacroAssembler::isStackRelated(result))
    89             result = static_cast<RegisterID>(result + 1);
    90         return result;
    91     }
    92    
    93     static RegisterID secondRealRegister()
    94     {
    95         return nextRegister(firstRealRegister());
     74        return static_cast<RegisterID>(reg + 1);
    9675    }
    9776   

  • trunk/Source/JavaScriptCore/ftl/FTLLocation.cpp

    r164333 r165196  
    3030
    3131#include "FTLSaveRestore.h"
     32#include "RegisterSet.h"
    3233#include <wtf/CommaPrinter.h>
    3334#include <wtf/DataLog.h>
    … …  
    158159void Location::restoreInto(MacroAssembler& jit, char* savedRegisters, GPRReg result, unsigned numFramesToPop) const
    159160{
    160     if (involvesGPR() && MacroAssembler::isStackRelated(gpr())) {
     161    if (involvesGPR() && RegisterSet::stackRegisters().get(gpr())) {
    161162        // Make the result GPR contain the appropriate stack register.
    162163        if (numFramesToPop) {
    … …  
    175176   
    176177    if (isGPR()) {
    177         if (MacroAssembler::isStackRelated(gpr())) {
     178        if (RegisterSet::stackRegisters().get(gpr())) {
    178179            // Already restored into result.
    179180        } else
    … …  
    198199       
    199200    case Indirect:
    200         if (MacroAssembler::isStackRelated(gpr())) {
     201        if (RegisterSet::stackRegisters().get(gpr())) {
    201202            // The stack register is already recovered into result.
    202203            jit.load64(MacroAssembler::Address(result, offset()), result);

  • trunk/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp

    r164326 r165196  
    11/*
    2  * Copyright (C) 2013 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3232#include "GPRInfo.h"
    3333#include "MacroAssembler.h"
     34#include "RegisterSet.h"
    3435
    3536namespace JSC { namespace FTL {
    … …  
    7071}
    7172
     73namespace {
     74
     75struct Regs {
     76    Regs()
     77    {
     78        special = RegisterSet::stackRegisters();
     79        special.merge(RegisterSet::reservedHardwareRegisters());
     80       
     81        first = MacroAssembler::firstRegister();
     82        while (special.get(first))
     83            first = MacroAssembler::nextRegister(first);
     84        second = MacroAssembler::nextRegister(first);
     85        while (special.get(second))
     86            second = MacroAssembler::nextRegister(second);
     87    }
     88   
     89    RegisterSet special;
     90    GPRReg first;
     91    GPRReg second;
     92};
     93
     94} // anonymous namespace
     95
    7296void saveAllRegisters(MacroAssembler& jit, char* scratchMemory)
    7397{
     98    Regs regs;
     99   
    74100    // Get the first register out of the way, so that we can use it as a pointer.
    75     jit.poke64(MacroAssembler::firstRealRegister(), 0);
    76     jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), MacroAssembler::firstRealRegister());
     101    jit.poke64(regs.first, 0);
     102    jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), regs.first);
    77103   
    78104    // Get all of the other GPRs out of the way.
    79     for (MacroAssembler::RegisterID reg = MacroAssembler::secondRealRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg))
    80         jit.store64(reg, MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(reg)));
     105    for (MacroAssembler::RegisterID reg = regs.second; reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
     106        if (regs.special.get(reg))
     107            continue;
     108        jit.store64(reg, MacroAssembler::Address(regs.first, offsetOfGPR(reg)));
     109    }
    81110   
    82111    // Restore the first register into the second one and save it.
    83     jit.peek64(MacroAssembler::secondRealRegister(), 0);
    84     jit.store64(MacroAssembler::secondRealRegister(), MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(MacroAssembler::firstRealRegister())));
     112    jit.peek64(regs.second, 0);
     113    jit.store64(regs.second, MacroAssembler::Address(regs.first, offsetOfGPR(regs.first)));
    85114   
    86115    // Finally save all FPR's.
    87     for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg))
    88         jit.storeDouble(reg, MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfFPR(reg)));
     116    for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
     117        if (regs.special.get(reg))
     118            continue;
     119        jit.storeDouble(reg, MacroAssembler::Address(regs.first, offsetOfFPR(reg)));
     120    }
    89121}
    90122
    91123void restoreAllRegisters(MacroAssembler& jit, char* scratchMemory)
    92124{
     125    Regs regs;
     126   
    93127    // Give ourselves a pointer to the scratch memory.
    94     jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), MacroAssembler::firstRealRegister());
     128    jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), regs.first);
    95129   
    96130    // Restore all FPR's.
    97     for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg))
    98         jit.loadDouble(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfFPR(reg)), reg);
     131    for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
     132        if (regs.special.get(reg))
     133            continue;
     134        jit.loadDouble(MacroAssembler::Address(regs.first, offsetOfFPR(reg)), reg);
     135    }
    99136   
    100     for (MacroAssembler::RegisterID reg = MacroAssembler::secondRealRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg))
    101         jit.load64(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(reg)), reg);
     137    for (MacroAssembler::RegisterID reg = regs.second; reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
     138        if (regs.special.get(reg))
     139            continue;
     140        jit.load64(MacroAssembler::Address(regs.first, offsetOfGPR(reg)), reg);
     141    }
    102142   
    103     jit.load64(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(MacroAssembler::firstRealRegister())), MacroAssembler::firstRealRegister());
     143    jit.load64(MacroAssembler::Address(regs.first, offsetOfGPR(regs.first)), regs.first);
    104144}
    105145

  • trunk/Source/JavaScriptCore/ftl/FTLSlowPathCall.cpp

    r163844 r165196  
    5353        , m_returnRegister(returnRegister)
    5454    {
    55         // We don't care that you're using callee-save or stack registers.
     55        // We don't care that you're using callee-save, stack, or hardware registers.
    5656        m_usedRegisters.exclude(RegisterSet::stackRegisters());
     57        m_usedRegisters.exclude(RegisterSet::reservedHardwareRegisters());
    5758        m_usedRegisters.exclude(RegisterSet::calleeSaveRegisters());
    5859       

  • trunk/Source/JavaScriptCore/jit/RegisterSet.cpp

    r164241 r165196  
    4343}
    4444
     45RegisterSet RegisterSet::reservedHardwareRegisters()
     46{
     47    RegisterSet result;
     48#if CPU(ARM64)
     49    result.set(ARM64Registers::lr);
     50#endif
     51    return result;
     52}
     53
     54RegisterSet RegisterSet::runtimeRegisters()
     55{
     56    RegisterSet result;
     57#if USE(JSVALUE64)
     58    result.set(GPRInfo::tagTypeNumberRegister);
     59    result.set(GPRInfo::tagMaskRegister);
     60#endif
     61    return result;
     62}
     63
    4564RegisterSet RegisterSet::specialRegisters()
    4665{
    4766    RegisterSet result;
    4867    result.merge(stackRegisters());
    49     result.set(GPRInfo::callFrameRegister);
    50 #if USE(JSVALUE64)
    51     result.set(GPRInfo::tagTypeNumberRegister);
    52     result.set(GPRInfo::tagMaskRegister);
    53 #endif
    54 #if CPU(ARM64)
    55     result.set(ARM64Registers::lr);
    56 #endif
     68    result.merge(reservedHardwareRegisters());
     69    result.merge(runtimeRegisters());
    5770    return result;
    5871}
    … …  
    7083#elif CPU(ARM64)
    7184    // We don't include LR in the set of callee-save registers even though it technically belongs
    72     // there. But, the way we use this list, it makes no sense to have it there.
     85    // there. This is because we use this set to describe the set of registers that need to be saved
     86    // beyond what you would save by the platform-agnostic "preserve return address" and "restore
     87    // return address" operations in CCallHelpers.
    7388    for (
    7489        ARM64Registers::RegisterID reg = ARM64Registers::x19;

  • trunk/Source/JavaScriptCore/jit/RegisterSet.h

    r164424 r165196  
    4343   
    4444    static RegisterSet stackRegisters();
    45     static RegisterSet specialRegisters();
     45    static RegisterSet reservedHardwareRegisters();
     46    static RegisterSet runtimeRegisters();
     47    static RegisterSet specialRegisters(); // The union of stack, reserved hardware, and runtime registers.
    4648    static RegisterSet calleeSaveRegisters();
    4749    static RegisterSet allGPRs();
Note: See TracChangeset for help on using the changeset viewer.