Skip to content

Commit 1e0aa7a

Browse files
gezalorewebkit-commit-queue
gezalore
authored and
webkit-commit-queue
committed
[JSC][32bit] Fix CSR restore on DFG tail calls, add extra register on ARMv7
https://bugs.webkit.org/show_bug.cgi?id=230622 Patch by Geza Lore <glore@igalia.com> on 2021-10-27 Reviewed by Keith Miller. This re-introduces the patch reverted by https://trac.webkit.org/changeset/284911/webkit with the C_LOOP interpreter now fixed. The only difference between the original patch and this version is in LowLevelInterpreter32_64.asm and LowLevelInterpreter64.asm, which need the PC base (PB) register restored on C_LOOP on return from a call, as C_LOOP does not seem to handle this as a proper callee save register (CSR). On non C_LOOP builds, the CSR restore mechanism takes care of this, so removed the superfluous instructions. --- Original ChangeLog --- This patch does two things: 1. Adds an extra callee save register (CSR) to be available to DFG on ARMv7. To do this properly required the following: 2. Implements the necessary shuffling in CallFrameShuffler on 32-bit architectures that is required to restore CSRs properly after a tail call on these architectures. This also fixes the remaining failures in the 32-bit build of the unlinked baseline JIT. * bytecode/ValueRecovery.cpp: (JSC::ValueRecovery::dumpInContext const): * bytecode/ValueRecovery.h: (JSC::ValueRecovery::calleeSaveRegDisplacedInJSStack): (JSC::ValueRecovery::isInJSStack const): (JSC::ValueRecovery::dataFormat const): (JSC::ValueRecovery::withLocalsOffset const): * dfg/DFGSpeculativeJIT32_64.cpp: (JSC::DFG::SpeculativeJIT::emitCall): * jit/CachedRecovery.cpp: (JSC::CachedRecovery::loadsIntoGPR const): * jit/CallFrameShuffleData.cpp: (JSC::CallFrameShuffleData::setupCalleeSaveRegisters): * jit/CallFrameShuffleData.h: * jit/CallFrameShuffler.cpp: (JSC::CallFrameShuffler::CallFrameShuffler): * jit/CallFrameShuffler.h: (JSC::CallFrameShuffler::snapshot const): (JSC::CallFrameShuffler::addNew): * jit/CallFrameShuffler32_64.cpp: (JSC::CallFrameShuffler::emitLoad): (JSC::CallFrameShuffler::emitDisplace): * jit/GPRInfo.h: (JSC::GPRInfo::toRegister): (JSC::GPRInfo::toIndex): * jit/RegisterSet.cpp: (JSC::RegisterSet::dfgCalleeSaveRegisters): * llint/LowLevelInterpreter32_64.asm: * llint/LowLevelInterpreter64.asm: Canonical link: https://commits.webkit.org/243594@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284923 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent ceb5f58 commit 1e0aa7a

14 files changed

Lines changed: 185 additions & 43 deletions

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,63 @@
1+
2021-10-27 Geza Lore <glore@igalia.com>
2+
3+
[JSC][32bit] Fix CSR restore on DFG tail calls, add extra register on ARMv7
4+
https://bugs.webkit.org/show_bug.cgi?id=230622
5+
6+
Reviewed by Keith Miller.
7+
8+
This re-introduces the patch reverted by
9+
https://trac.webkit.org/changeset/284911/webkit
10+
with the C_LOOP interpreter now fixed.
11+
12+
The only difference between the original patch and this version is in
13+
LowLevelInterpreter32_64.asm and LowLevelInterpreter64.asm, which
14+
need the PC base (PB) register restored on C_LOOP on return from a
15+
call, as C_LOOP does not seem to handle this as a proper callee save
16+
register (CSR). On non C_LOOP builds, the CSR restore mechanism takes
17+
care of this, so removed the superfluous instructions.
18+
19+
--- Original ChangeLog ---
20+
21+
This patch does two things:
22+
23+
1. Adds an extra callee save register (CSR) to be available to DFG on
24+
ARMv7. To do this properly required the following:
25+
26+
2. Implements the necessary shuffling in CallFrameShuffler on 32-bit
27+
architectures that is required to restore CSRs properly after a tail
28+
call on these architectures. This also fixes the remaining failures in
29+
the 32-bit build of the unlinked baseline JIT.
30+
31+
* bytecode/ValueRecovery.cpp:
32+
(JSC::ValueRecovery::dumpInContext const):
33+
* bytecode/ValueRecovery.h:
34+
(JSC::ValueRecovery::calleeSaveRegDisplacedInJSStack):
35+
(JSC::ValueRecovery::isInJSStack const):
36+
(JSC::ValueRecovery::dataFormat const):
37+
(JSC::ValueRecovery::withLocalsOffset const):
38+
* dfg/DFGSpeculativeJIT32_64.cpp:
39+
(JSC::DFG::SpeculativeJIT::emitCall):
40+
* jit/CachedRecovery.cpp:
41+
(JSC::CachedRecovery::loadsIntoGPR const):
42+
* jit/CallFrameShuffleData.cpp:
43+
(JSC::CallFrameShuffleData::setupCalleeSaveRegisters):
44+
* jit/CallFrameShuffleData.h:
45+
* jit/CallFrameShuffler.cpp:
46+
(JSC::CallFrameShuffler::CallFrameShuffler):
47+
* jit/CallFrameShuffler.h:
48+
(JSC::CallFrameShuffler::snapshot const):
49+
(JSC::CallFrameShuffler::addNew):
50+
* jit/CallFrameShuffler32_64.cpp:
51+
(JSC::CallFrameShuffler::emitLoad):
52+
(JSC::CallFrameShuffler::emitDisplace):
53+
* jit/GPRInfo.h:
54+
(JSC::GPRInfo::toRegister):
55+
(JSC::GPRInfo::toIndex):
56+
* jit/RegisterSet.cpp:
57+
(JSC::RegisterSet::dfgCalleeSaveRegisters):
58+
* llint/LowLevelInterpreter32_64.asm:
59+
* llint/LowLevelInterpreter64.asm:
60+
161
2021-10-26 Commit Queue <commit-queue@webkit.org>
262

363
Unreviewed, reverting r284255.

‎Source/JavaScriptCore/bytecode/ValueRecovery.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,11 @@ void ValueRecovery::dumpInContext(PrintStream& out, DumpContext* context) const
9999
case Int32DisplacedInJSStack:
100100
out.print("*int32(", virtualRegister(), ")");
101101
return;
102+
#if USE(JSVALUE32_64)
103+
case Int32TagDisplacedInJSStack:
104+
out.print("*int32Tag(", virtualRegister(), ")");
105+
return;
106+
#endif
102107
case Int52DisplacedInJSStack:
103108
out.print("*int52(", virtualRegister(), ")");
104109
return;

‎Source/JavaScriptCore/bytecode/ValueRecovery.h‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ enum ValueRecoveryTechnique : uint8_t {
6060
DisplacedInJSStack,
6161
// It's in the stack, at a different location, and it's unboxed.
6262
Int32DisplacedInJSStack,
63+
#if USE(JSVALUE32_64)
64+
Int32TagDisplacedInJSStack, // int32 stored in tag field
65+
#endif
6366
Int52DisplacedInJSStack,
6467
StrictInt52DisplacedInJSStack,
6568
DoubleDisplacedInJSStack,
@@ -187,7 +190,19 @@ class ValueRecovery {
187190
result.m_source = WTFMove(u);
188191
return result;
189192
}
190-
193+
194+
#if USE(JSVALUE32_64)
195+
static ValueRecovery calleeSaveRegDisplacedInJSStack(VirtualRegister virtualReg, bool inTag)
196+
{
197+
ValueRecovery result;
198+
UnionType u;
199+
u.virtualReg = virtualReg.offset();
200+
result.m_source = WTFMove(u);
201+
result.m_technique = inTag ? Int32TagDisplacedInJSStack : Int32DisplacedInJSStack;
202+
return result;
203+
}
204+
#endif
205+
191206
static ValueRecovery constant(JSValue value)
192207
{
193208
ValueRecovery result;
@@ -258,6 +273,9 @@ class ValueRecovery {
258273
switch (m_technique) {
259274
case DisplacedInJSStack:
260275
case Int32DisplacedInJSStack:
276+
#if USE(JSVALUE32_64)
277+
case Int32TagDisplacedInJSStack:
278+
#endif
261279
case Int52DisplacedInJSStack:
262280
case StrictInt52DisplacedInJSStack:
263281
case DoubleDisplacedInJSStack:
@@ -282,6 +300,9 @@ class ValueRecovery {
282300
return DataFormatJS;
283301
case UnboxedInt32InGPR:
284302
case Int32DisplacedInJSStack:
303+
#if USE(JSVALUE32_64)
304+
case Int32TagDisplacedInJSStack:
305+
#endif
285306
return DataFormatInt32;
286307
case UnboxedInt52InGPR:
287308
case Int52DisplacedInJSStack:
@@ -358,6 +379,9 @@ class ValueRecovery {
358379
switch (m_technique) {
359380
case DisplacedInJSStack:
360381
case Int32DisplacedInJSStack:
382+
#if USE(JSVALUE32_64)
383+
case Int32TagDisplacedInJSStack:
384+
#endif
361385
case DoubleDisplacedInJSStack:
362386
case CellDisplacedInJSStack:
363387
case BooleanDisplacedInJSStack:

‎Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,8 @@ void SpeculativeJIT::emitCall(Node* node)
742742

743743
for (unsigned i = numPassedArgs; i < numAllocatedArgs; ++i)
744744
shuffleData.args[i] = ValueRecovery::constant(jsUndefined());
745+
746+
shuffleData.setupCalleeSaveRegisters(m_jit.codeBlock());
745747
} else {
746748
m_jit.store32(MacroAssembler::TrustedImm32(numPassedArgs), m_jit.calleeFramePayloadSlot(CallFrameSlot::argumentCountIncludingThis));
747749

‎Source/JavaScriptCore/jit/CachedRecovery.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ bool CachedRecovery::loadsIntoGPR() const
5252
{
5353
switch (recovery().technique()) {
5454
case Int32DisplacedInJSStack:
55-
#if USE(JSVALUE64)
55+
#if USE(JSVALUE32_64)
56+
case Int32TagDisplacedInJSStack:
57+
#elif USE(JSVALUE64)
5658
case Int52DisplacedInJSStack:
5759
case StrictInt52DisplacedInJSStack:
5860
case DisplacedInJSStack:

‎Source/JavaScriptCore/jit/CallFrameShuffleData.cpp‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@
3333

3434
namespace JSC {
3535

36-
#if USE(JSVALUE64)
37-
3836
void CallFrameShuffleData::setupCalleeSaveRegisters(CodeBlock* codeBlock)
3937
{
4038
setupCalleeSaveRegisters(codeBlock->calleeSaveRegisters());
@@ -49,9 +47,24 @@ void CallFrameShuffleData::setupCalleeSaveRegisters(const RegisterAtOffsetList*
4947
if (!calleeSaveRegisters.get(entry.reg()))
5048
continue;
5149

52-
VirtualRegister saveSlot { entry.offsetAsIndex() };
50+
int saveSlotIndexInCPURegisters = entry.offsetAsIndex();
51+
52+
#if USE(JSVALUE64)
53+
// CPU registers are the same size as virtual registers
54+
VirtualRegister saveSlot { saveSlotIndexInCPURegisters };
5355
registers[entry.reg()]
5456
= ValueRecovery::displacedInJSStack(saveSlot, DataFormatJS);
57+
#elif USE(JSVALUE32_64)
58+
// On 32-bit architectures, 2 callee saved registers may be packed into the same slot
59+
static_assert(!PayloadOffset || !TagOffset);
60+
static_assert(PayloadOffset == 4 || TagOffset == 4);
61+
bool inTag = (saveSlotIndexInCPURegisters & 1) == !!TagOffset;
62+
if (saveSlotIndexInCPURegisters < 0)
63+
saveSlotIndexInCPURegisters -= 1; // Round towards -inf
64+
VirtualRegister saveSlot { saveSlotIndexInCPURegisters / 2 };
65+
registers[entry.reg()]
66+
= ValueRecovery::calleeSaveRegDisplacedInJSStack(saveSlot, inTag);
67+
#endif
5568
}
5669

5770
for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {
@@ -61,12 +74,14 @@ void CallFrameShuffleData::setupCalleeSaveRegisters(const RegisterAtOffsetList*
6174
if (registers[reg])
6275
continue;
6376

77+
#if USE(JSVALUE64)
6478
registers[reg] = ValueRecovery::inRegister(reg, DataFormatJS);
79+
#elif USE(JSVALUE32_64)
80+
registers[reg] = ValueRecovery::inRegister(reg, DataFormatInt32);
81+
#endif
6582
}
6683
}
6784

68-
#endif // USE(JSVALUE64)
69-
7085
} // namespace JSC
7186

7287
#endif // ENABLE(JIT)

‎Source/JavaScriptCore/jit/CallFrameShuffleData.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,13 @@ struct CallFrameShuffleData {
4444
unsigned numLocals { UINT_MAX };
4545
unsigned numPassedArgs { UINT_MAX };
4646
unsigned numParameters { UINT_MAX }; // On our machine frame.
47-
#if USE(JSVALUE64)
4847
RegisterMap<ValueRecovery> registers;
48+
#if USE(JSVALUE64)
4949
GPRReg numberTagRegister { InvalidGPRReg };
50+
#endif
5051

5152
void setupCalleeSaveRegisters(CodeBlock*);
5253
void setupCalleeSaveRegisters(const RegisterAtOffsetList*);
53-
#endif
5454
ValueRecovery callee;
5555
};
5656

‎Source/JavaScriptCore/jit/CallFrameShuffler.cpp‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,8 @@ CallFrameShuffler::CallFrameShuffler(CCallHelpers& jit, const CallFrameShuffleDa
5252
for (unsigned i = FPRInfo::numberOfRegisters; i--; )
5353
m_lockedRegisters.clear(FPRInfo::toRegister(i));
5454

55-
#if USE(JSVALUE64)
56-
// ... as well as the runtime registers on 64-bit architectures.
57-
// However do not use these registers on 32-bit architectures since
58-
// saving and restoring callee-saved registers in CallFrameShuffler isn't supported
59-
// on 32-bit architectures yet.
55+
// ... as well as the callee saved registers
6056
m_lockedRegisters.exclude(RegisterSet::vmCalleeSaveRegisters());
61-
#endif
6257

6358
ASSERT(!data.callee.isInJSStack() || data.callee.virtualRegister().isLocal());
6459
addNew(CallFrameSlot::callee, data.callee);
@@ -68,17 +63,21 @@ CallFrameShuffler::CallFrameShuffler(CCallHelpers& jit, const CallFrameShuffleDa
6863
addNew(virtualRegisterForArgumentIncludingThis(i), data.args[i]);
6964
}
7065

71-
#if USE(JSVALUE64)
7266
for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {
7367
if (!data.registers[reg].isSet())
7468
continue;
7569

76-
if (reg.isGPR())
70+
if (reg.isGPR()) {
71+
#if USE(JSVALUE64)
7772
addNew(JSValueRegs(reg.gpr()), data.registers[reg]);
78-
else
73+
#elif USE(JSVALUE32_64)
74+
addNew(reg.gpr(), data.registers[reg]);
75+
#endif
76+
} else
7977
addNew(reg.fpr(), data.registers[reg]);
8078
}
8179

80+
#if USE(JSVALUE64)
8281
m_numberTagRegister = data.numberTagRegister;
8382
if (m_numberTagRegister != InvalidGPRReg)
8483
lockGPR(m_numberTagRegister);

‎Source/JavaScriptCore/jit/CallFrameShuffler.h‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,15 @@ class CallFrameShuffler {
117117

118118
#if USE(JSVALUE64)
119119
data.registers[reg] = cachedRecovery->recovery();
120+
#elif USE(JSVALUE32_64)
121+
ValueRecovery recovery = cachedRecovery->recovery();
122+
if (recovery.technique() == DisplacedInJSStack) {
123+
JSValueRegs wantedJSValueReg = cachedRecovery->wantedJSValueRegs();
124+
ASSERT(reg == wantedJSValueReg.payloadGPR() || reg == wantedJSValueReg.tagGPR());
125+
bool inTag = reg == wantedJSValueReg.tagGPR();
126+
data.registers[reg] = ValueRecovery::calleeSaveRegDisplacedInJSStack(recovery.virtualRegister(), inTag);
127+
} else
128+
data.registers[reg] = recovery;
120129
#else
121130
RELEASE_ASSERT_NOT_REACHED();
122131
#endif
@@ -664,6 +673,32 @@ class CallFrameShuffler {
664673
cachedRecovery->setWantedJSValueRegs(jsValueRegs);
665674
}
666675

676+
#if USE(JSVALUE32_64)
677+
void addNew(GPRReg gpr, ValueRecovery recovery)
678+
{
679+
ASSERT(gpr != InvalidGPRReg && !m_newRegisters[gpr]);
680+
ASSERT(recovery.technique() == Int32DisplacedInJSStack
681+
|| recovery.technique() == Int32TagDisplacedInJSStack);
682+
CachedRecovery* cachedRecovery = addCachedRecovery(recovery);
683+
if (JSValueRegs oldRegs { cachedRecovery->wantedJSValueRegs() }) {
684+
// Combine with the other CSR in the same virtual register slot
685+
ASSERT(oldRegs.tagGPR() == InvalidGPRReg);
686+
ASSERT(oldRegs.payloadGPR() != InvalidGPRReg && oldRegs.payloadGPR() != gpr);
687+
if (recovery.technique() == Int32DisplacedInJSStack) {
688+
ASSERT(cachedRecovery->recovery().technique() == Int32TagDisplacedInJSStack);
689+
cachedRecovery->setWantedJSValueRegs(JSValueRegs(oldRegs.payloadGPR(), gpr));
690+
} else {
691+
ASSERT(cachedRecovery->recovery().technique() == Int32DisplacedInJSStack);
692+
cachedRecovery->setWantedJSValueRegs(JSValueRegs(gpr, oldRegs.payloadGPR()));
693+
}
694+
cachedRecovery->setRecovery(
695+
ValueRecovery::displacedInJSStack(recovery.virtualRegister(), DataFormatJS));
696+
} else
697+
cachedRecovery->setWantedJSValueRegs(JSValueRegs::payloadOnly(gpr));
698+
m_newRegisters[gpr] = cachedRecovery;
699+
}
700+
#endif
701+
667702
void addNew(FPRReg fpr, ValueRecovery recovery)
668703
{
669704
ASSERT(fpr != InvalidFPRReg && !m_newRegisters[fpr]);

‎Source/JavaScriptCore/jit/CallFrameShuffler32_64.cpp‎

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,11 @@ void CallFrameShuffler::emitLoad(CachedRecovery& location)
124124
if (resultGPR == InvalidGPRReg || m_registers[resultGPR] || m_lockedRegisters.get(resultGPR))
125125
resultGPR = getFreeGPR();
126126
ASSERT(resultGPR != InvalidGPRReg);
127-
m_jit.loadPtr(address.withOffset(PayloadOffset), resultGPR);
128-
updateRecovery(location,
127+
if (location.recovery().technique() == Int32TagDisplacedInJSStack)
128+
m_jit.loadPtr(address.withOffset(TagOffset), resultGPR);
129+
else
130+
m_jit.loadPtr(address.withOffset(PayloadOffset), resultGPR);
131+
updateRecovery(location,
129132
ValueRecovery::inGPR(resultGPR, location.recovery().dataFormat()));
130133
if (verbose)
131134
dataLog(location.recovery(), "\n");
@@ -190,28 +193,18 @@ void CallFrameShuffler::emitDisplace(CachedRecovery& location)
190193
if (wantedTagGPR != InvalidGPRReg) {
191194
ASSERT(!m_lockedRegisters.get(wantedTagGPR));
192195
if (CachedRecovery* currentTag { m_registers[wantedTagGPR] }) {
193-
if (currentTag == &location) {
194-
if (verbose)
195-
dataLog(" + ", wantedTagGPR, " is OK\n");
196-
} else {
197-
// This can never happen on 32bit platforms since we
198-
// have at most one wanted JSValueRegs, for the
199-
// callee, and no callee-save registers.
200-
RELEASE_ASSERT_NOT_REACHED();
201-
}
196+
RELEASE_ASSERT(currentTag == &location);
197+
if (verbose)
198+
dataLog(" + ", wantedTagGPR, " is OK\n");
202199
}
203200
}
204201

205202
if (wantedPayloadGPR != InvalidGPRReg) {
206203
ASSERT(!m_lockedRegisters.get(wantedPayloadGPR));
207204
if (CachedRecovery* currentPayload { m_registers[wantedPayloadGPR] }) {
208-
if (currentPayload == &location) {
209-
if (verbose)
210-
dataLog(" + ", wantedPayloadGPR, " is OK\n");
211-
} else {
212-
// See above
213-
RELEASE_ASSERT_NOT_REACHED();
214-
}
205+
RELEASE_ASSERT(currentPayload == &location);
206+
if (verbose)
207+
dataLog(" + ", wantedPayloadGPR, " is OK\n");
215208
}
216209
}
217210

0 commit comments

Comments
 (0)