- Revision
- 165196
- Author
- [email protected]
- Date
- 2014-03-06 10:33:18 -0800 (Thu, 06 Mar 2014)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (165195 => 165196)
--- trunk/Source/_javascript_Core/ChangeLog 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-03-06 18:33:18 UTC (rev 165196)
@@ -1,5 +1,49 @@
2014-03-06 Filip Pizlo <[email protected]>
+ 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:
+
+2014-03-06 Filip Pizlo <[email protected]>
+
Unreviewed, fix build.
* disassembler/ARM64Disassembler.cpp:
Modified: trunk/Source/_javascript_Core/assembler/ARM64Assembler.h (165195 => 165196)
--- trunk/Source/_javascript_Core/assembler/ARM64Assembler.h 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/assembler/ARM64Assembler.h 2014-03-06 18:33:18 UTC (rev 165196)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2014 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -478,7 +478,7 @@
typedef ARM64Registers::FPRegisterID FPRegisterID;
static RegisterID firstRegister() { return ARM64Registers::x0; }
- static RegisterID lastRegister() { return ARM64Registers::x28; }
+ static RegisterID lastRegister() { return ARM64Registers::sp; }
static FPRegisterID firstFPRegister() { return ARM64Registers::q0; }
static FPRegisterID lastFPRegister() { return ARM64Registers::q31; }
Modified: trunk/Source/_javascript_Core/assembler/MacroAssembler.h (165195 => 165196)
--- trunk/Source/_javascript_Core/assembler/MacroAssembler.h 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/assembler/MacroAssembler.h 2014-03-06 18:33:18 UTC (rev 165196)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2012, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2008, 2012, 2013, 2014 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -69,32 +69,11 @@
class MacroAssembler : public MacroAssemblerBase {
public:
- static bool isStackRelated(RegisterID reg)
- {
- return reg == stackPointerRegister || reg == framePointerRegister;
- }
-
- static RegisterID firstRealRegister()
- {
- RegisterID firstRegister = MacroAssembler::firstRegister();
- while (MacroAssembler::isStackRelated(firstRegister))
- firstRegister = static_cast<RegisterID>(firstRegister + 1);
- return firstRegister;
- }
-
static RegisterID nextRegister(RegisterID reg)
{
- RegisterID result = static_cast<RegisterID>(reg + 1);
- while (MacroAssembler::isStackRelated(result))
- result = static_cast<RegisterID>(result + 1);
- return result;
+ return static_cast<RegisterID>(reg + 1);
}
- static RegisterID secondRealRegister()
- {
- return nextRegister(firstRealRegister());
- }
-
static FPRegisterID nextFPRegister(FPRegisterID reg)
{
return static_cast<FPRegisterID>(reg + 1);
Modified: trunk/Source/_javascript_Core/ftl/FTLLocation.cpp (165195 => 165196)
--- trunk/Source/_javascript_Core/ftl/FTLLocation.cpp 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/ftl/FTLLocation.cpp 2014-03-06 18:33:18 UTC (rev 165196)
@@ -29,6 +29,7 @@
#if ENABLE(FTL_JIT)
#include "FTLSaveRestore.h"
+#include "RegisterSet.h"
#include <wtf/CommaPrinter.h>
#include <wtf/DataLog.h>
#include <wtf/ListDump.h>
@@ -157,7 +158,7 @@
void Location::restoreInto(MacroAssembler& jit, char* savedRegisters, GPRReg result, unsigned numFramesToPop) const
{
- if (involvesGPR() && MacroAssembler::isStackRelated(gpr())) {
+ if (involvesGPR() && RegisterSet::stackRegisters().get(gpr())) {
// Make the result GPR contain the appropriate stack register.
if (numFramesToPop) {
jit.move(MacroAssembler::framePointerRegister, result);
@@ -174,7 +175,7 @@
}
if (isGPR()) {
- if (MacroAssembler::isStackRelated(gpr())) {
+ if (RegisterSet::stackRegisters().get(gpr())) {
// Already restored into result.
} else
jit.load64(savedRegisters + offsetOfGPR(gpr()), result);
@@ -197,7 +198,7 @@
return;
case Indirect:
- if (MacroAssembler::isStackRelated(gpr())) {
+ if (RegisterSet::stackRegisters().get(gpr())) {
// The stack register is already recovered into result.
jit.load64(MacroAssembler::Address(result, offset()), result);
return;
Modified: trunk/Source/_javascript_Core/ftl/FTLSaveRestore.cpp (165195 => 165196)
--- trunk/Source/_javascript_Core/ftl/FTLSaveRestore.cpp 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/ftl/FTLSaveRestore.cpp 2014-03-06 18:33:18 UTC (rev 165196)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -31,6 +31,7 @@
#include "FPRInfo.h"
#include "GPRInfo.h"
#include "MacroAssembler.h"
+#include "RegisterSet.h"
namespace JSC { namespace FTL {
@@ -69,38 +70,77 @@
return offsetOfFPR(reg.fpr());
}
+namespace {
+
+struct Regs {
+ Regs()
+ {
+ special = RegisterSet::stackRegisters();
+ special.merge(RegisterSet::reservedHardwareRegisters());
+
+ first = MacroAssembler::firstRegister();
+ while (special.get(first))
+ first = MacroAssembler::nextRegister(first);
+ second = MacroAssembler::nextRegister(first);
+ while (special.get(second))
+ second = MacroAssembler::nextRegister(second);
+ }
+
+ RegisterSet special;
+ GPRReg first;
+ GPRReg second;
+};
+
+} // anonymous namespace
+
void saveAllRegisters(MacroAssembler& jit, char* scratchMemory)
{
+ Regs regs;
+
// Get the first register out of the way, so that we can use it as a pointer.
- jit.poke64(MacroAssembler::firstRealRegister(), 0);
- jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), MacroAssembler::firstRealRegister());
+ jit.poke64(regs.first, 0);
+ jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), regs.first);
// Get all of the other GPRs out of the way.
- for (MacroAssembler::RegisterID reg = MacroAssembler::secondRealRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg))
- jit.store64(reg, MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(reg)));
+ for (MacroAssembler::RegisterID reg = regs.second; reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
+ if (regs.special.get(reg))
+ continue;
+ jit.store64(reg, MacroAssembler::Address(regs.first, offsetOfGPR(reg)));
+ }
// Restore the first register into the second one and save it.
- jit.peek64(MacroAssembler::secondRealRegister(), 0);
- jit.store64(MacroAssembler::secondRealRegister(), MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(MacroAssembler::firstRealRegister())));
+ jit.peek64(regs.second, 0);
+ jit.store64(regs.second, MacroAssembler::Address(regs.first, offsetOfGPR(regs.first)));
// Finally save all FPR's.
- for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg))
- jit.storeDouble(reg, MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfFPR(reg)));
+ for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
+ if (regs.special.get(reg))
+ continue;
+ jit.storeDouble(reg, MacroAssembler::Address(regs.first, offsetOfFPR(reg)));
+ }
}
void restoreAllRegisters(MacroAssembler& jit, char* scratchMemory)
{
+ Regs regs;
+
// Give ourselves a pointer to the scratch memory.
- jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), MacroAssembler::firstRealRegister());
+ jit.move(MacroAssembler::TrustedImmPtr(scratchMemory), regs.first);
// Restore all FPR's.
- for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg))
- jit.loadDouble(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfFPR(reg)), reg);
+ for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
+ if (regs.special.get(reg))
+ continue;
+ jit.loadDouble(MacroAssembler::Address(regs.first, offsetOfFPR(reg)), reg);
+ }
- for (MacroAssembler::RegisterID reg = MacroAssembler::secondRealRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg))
- jit.load64(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(reg)), reg);
+ for (MacroAssembler::RegisterID reg = regs.second; reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
+ if (regs.special.get(reg))
+ continue;
+ jit.load64(MacroAssembler::Address(regs.first, offsetOfGPR(reg)), reg);
+ }
- jit.load64(MacroAssembler::Address(MacroAssembler::firstRealRegister(), offsetOfGPR(MacroAssembler::firstRealRegister())), MacroAssembler::firstRealRegister());
+ jit.load64(MacroAssembler::Address(regs.first, offsetOfGPR(regs.first)), regs.first);
}
} } // namespace JSC::FTL
Modified: trunk/Source/_javascript_Core/ftl/FTLSlowPathCall.cpp (165195 => 165196)
--- trunk/Source/_javascript_Core/ftl/FTLSlowPathCall.cpp 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/ftl/FTLSlowPathCall.cpp 2014-03-06 18:33:18 UTC (rev 165196)
@@ -52,8 +52,9 @@
, m_numArgs(numArgs)
, m_returnRegister(returnRegister)
{
- // We don't care that you're using callee-save or stack registers.
+ // We don't care that you're using callee-save, stack, or hardware registers.
m_usedRegisters.exclude(RegisterSet::stackRegisters());
+ m_usedRegisters.exclude(RegisterSet::reservedHardwareRegisters());
m_usedRegisters.exclude(RegisterSet::calleeSaveRegisters());
// The return register doesn't need to be saved.
Modified: trunk/Source/_javascript_Core/jit/RegisterSet.cpp (165195 => 165196)
--- trunk/Source/_javascript_Core/jit/RegisterSet.cpp 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/jit/RegisterSet.cpp 2014-03-06 18:33:18 UTC (rev 165196)
@@ -42,21 +42,34 @@
return result;
}
-RegisterSet RegisterSet::specialRegisters()
+RegisterSet RegisterSet::reservedHardwareRegisters()
{
RegisterSet result;
- result.merge(stackRegisters());
- result.set(GPRInfo::callFrameRegister);
+#if CPU(ARM64)
+ result.set(ARM64Registers::lr);
+#endif
+ return result;
+}
+
+RegisterSet RegisterSet::runtimeRegisters()
+{
+ RegisterSet result;
#if USE(JSVALUE64)
result.set(GPRInfo::tagTypeNumberRegister);
result.set(GPRInfo::tagMaskRegister);
#endif
-#if CPU(ARM64)
- result.set(ARM64Registers::lr);
-#endif
return result;
}
+RegisterSet RegisterSet::specialRegisters()
+{
+ RegisterSet result;
+ result.merge(stackRegisters());
+ result.merge(reservedHardwareRegisters());
+ result.merge(runtimeRegisters());
+ return result;
+}
+
RegisterSet RegisterSet::calleeSaveRegisters()
{
RegisterSet result;
@@ -69,7 +82,9 @@
result.set(X86Registers::r15);
#elif CPU(ARM64)
// We don't include LR in the set of callee-save registers even though it technically belongs
- // there. But, the way we use this list, it makes no sense to have it there.
+ // there. This is because we use this set to describe the set of registers that need to be saved
+ // beyond what you would save by the platform-agnostic "preserve return address" and "restore
+ // return address" operations in CCallHelpers.
for (
ARM64Registers::RegisterID reg = ARM64Registers::x19;
reg <= ARM64Registers::x28;
Modified: trunk/Source/_javascript_Core/jit/RegisterSet.h (165195 => 165196)
--- trunk/Source/_javascript_Core/jit/RegisterSet.h 2014-03-06 18:05:20 UTC (rev 165195)
+++ trunk/Source/_javascript_Core/jit/RegisterSet.h 2014-03-06 18:33:18 UTC (rev 165196)
@@ -42,7 +42,9 @@
RegisterSet() { }
static RegisterSet stackRegisters();
- static RegisterSet specialRegisters();
+ static RegisterSet reservedHardwareRegisters();
+ static RegisterSet runtimeRegisters();
+ static RegisterSet specialRegisters(); // The union of stack, reserved hardware, and runtime registers.
static RegisterSet calleeSaveRegisters();
static RegisterSet allGPRs();
static RegisterSet allFPRs();
