Skip to content

Commit 7cda7d7

Browse files
kmiller68kmiller68
committed
[JSC] Improve op_mod for int32 uses and add LOL support
https://bugs.webkit.org/show_bug.cgi?id=307683 rdar://170243169 Reviewed by Marcus Plutowski. Right now Baseline just goes to the slow path for ARM64 on op_mod. This patch adds a fast path for binary int32s (assuming the divisor is non-zero). Also, clean-up some of the DFG's equivalent code. Test: JSTests/stress/int32-mod-edge-cases.js Canonical link: https://commits.webkit.org/307425@main
1 parent 2b80848 commit 7cda7d7

8 files changed

Lines changed: 226 additions & 16 deletions

File tree

‎JSTests/stress/int32-mod-edge-cases.js‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
function shouldBe(actual, expected) {
2+
if (!Object.is(actual, expected))
3+
throw new Error('bad value: ' + actual + ', expected: ' + expected);
4+
}
5+
6+
function testMod(x, y) {
7+
return x % y;
8+
}
9+
noInline(testMod);
10+
11+
// Test negative zero case: when numerator is negative and remainder is 0
12+
function testNegativeZero() {
13+
// -0 % 1 should be -0
14+
shouldBe(testMod(-0, 1), -0);
15+
shouldBe(testMod(-0, 2), -0);
16+
shouldBe(testMod(-0, 10), -0);
17+
18+
// -4 % 2 should be -0 (evenly divisible)
19+
shouldBe(testMod(-4, 2), -0);
20+
shouldBe(testMod(-10, 5), -0);
21+
shouldBe(testMod(-100, 10), -0);
22+
}
23+
24+
// Test overflow case: when (x / y) * y would overflow
25+
function testOverflow() {
26+
// On ARM64, the multiply check can overflow for large values
27+
// INT32_MIN % -1 is a special case that should return -0
28+
shouldBe(testMod(-2147483648, -1), -0);
29+
30+
// Other cases near overflow boundaries
31+
shouldBe(testMod(2147483647, 2147483647), 0);
32+
shouldBe(testMod(-2147483648, 2147483647), -1);
33+
shouldBe(testMod(2147483647, -2147483648), 2147483647);
34+
}
35+
36+
// Test regular modulo operations to ensure we JIT compile
37+
function testRegularMod() {
38+
shouldBe(testMod(10, 3), 1);
39+
shouldBe(testMod(17, 5), 2);
40+
shouldBe(testMod(-10, 3), -1);
41+
shouldBe(testMod(-17, 5), -2);
42+
shouldBe(testMod(10, -3), 1);
43+
shouldBe(testMod(17, -5), 2);
44+
shouldBe(testMod(-10, -3), -1);
45+
shouldBe(testMod(-17, -5), -2);
46+
shouldBe(testMod(0, -1), 0);
47+
}
48+
49+
// Main test loop - ensures JIT compilation
50+
for (var i = 0; i < testLoopCount; ++i) {
51+
testRegularMod();
52+
testNegativeZero();
53+
testOverflow();
54+
}

‎Source/JavaScriptCore/assembler/ARMv7Assembler.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1719,7 +1719,7 @@ class ARMv7Assembler {
17191719
}
17201720

17211721
#if HAVE(ARM_IDIV_INSTRUCTIONS)
1722-
template<int datasize>
1722+
template<int datasize = 32>
17231723
ALWAYS_INLINE void sdiv(RegisterID rd, RegisterID rn, RegisterID rm)
17241724
{
17251725
static_assert(datasize == 32, "sdiv datasize must be 32 for armv7s");

‎Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,14 @@ class MacroAssemblerARMv7 : public AbstractMacroAssembler<Assembler> {
645645
m_assembler.umull(destLo, destHi, left, right);
646646
}
647647

648+
#if HAVE(ARM_IDIV_INSTRUCTIONS)
649+
// FIXME: Add more of div flavors.
650+
void div32(RegisterID left, RegisterID right, RegisterID dest)
651+
{
652+
m_assembler.sdiv(dest, left, right);
653+
}
654+
#endif
655+
648656
void neg32(RegisterID srcDest)
649657
{
650658
m_assembler.neg(srcDest, srcDest);

‎Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6314,13 +6314,10 @@ void SpeculativeJIT::compileArithMod(Node* node)
63146314
strictInt32Result(edx.gpr(), node);
63156315

63166316
#elif HAVE(ARM_IDIV_INSTRUCTIONS) || CPU(ARM64)
6317-
GPRTemporary temp(this);
63186317
GPRTemporary quotientThenRemainder(this);
6319-
GPRTemporary multiplyAnswer(this);
63206318
GPRReg dividendGPR = op1.gpr();
63216319
GPRReg divisorGPR = op2.gpr();
63226320
GPRReg quotientThenRemainderGPR = quotientThenRemainder.gpr();
6323-
GPRReg multiplyAnswerGPR = multiplyAnswer.gpr();
63246321

63256322
JumpList done;
63266323

@@ -6336,15 +6333,14 @@ void SpeculativeJIT::compileArithMod(Node* node)
63366333
denominatorNotZero.link(this);
63376334
}
63386335

6339-
assembler().sdiv<32>(quotientThenRemainderGPR, dividendGPR, divisorGPR);
6340-
// FIXME: It seems like there are cases where we don't need this? What if we have
6341-
// arithMode() == Arith::Unchecked?
6342-
// https://bugs.webkit.org/show_bug.cgi?id=126444
6343-
speculationCheck(ExitKind::Overflow, JSValueRegs(), 0, branchMul32(Overflow, quotientThenRemainderGPR, divisorGPR, multiplyAnswerGPR));
6344-
#if HAVE(ARM_IDIV_INSTRUCTIONS)
6345-
assembler().sub(quotientThenRemainderGPR, dividendGPR, multiplyAnswerGPR);
6336+
// This is doing: x - ((x / y) * y)
6337+
div32(dividendGPR, divisorGPR, quotientThenRemainderGPR);
6338+
// This should only overflow for INT32_MIN % -1 but that will end up with quotientThenRemainderGPR == 0, which will be handled as needed in the negative zero check.
6339+
#if CPU(ARM64)
6340+
multiplySub32(quotientThenRemainderGPR, divisorGPR, dividendGPR, quotientThenRemainderGPR);
63466341
#else
6347-
assembler().sub<32>(quotientThenRemainderGPR, dividendGPR, multiplyAnswerGPR);
6342+
mul32(quotientThenRemainderGPR, divisorGPR, quotientThenRemainderGPR);
6343+
sub32(dividendGPR, quotientThenRemainderGPR, quotientThenRemainderGPR);
63486344
#endif
63496345

63506346
// If the user cares about negative zero, then speculate that we're not about

‎Source/JavaScriptCore/jit/JITArithmetic.cpp‎

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ void JIT::emit_op_mod(const JSInstruction* currentInstruction)
503503
move(regT4, regT0);
504504
addSlowCase(branchTest32(Zero, ecx));
505505
Jump denominatorNotNeg1 = branch32(NotEqual, ecx, TrustedImm32(-1));
506-
addSlowCase(branch32(Equal, regT0, TrustedImm32(-2147483647-1)));
506+
addSlowCase(branch32(Equal, regT0, TrustedImm32(INT32_MIN)));
507507
denominatorNotNeg1.link(this);
508508
x86ConvertToDoubleWord32();
509509
x86Div32(ecx);
@@ -522,7 +522,56 @@ void JIT::emitSlow_op_mod(const JSInstruction*, Vector<SlowCaseEntry>::iterator&
522522
slowPathCall.call();
523523
}
524524

525-
#else // CPU(X86_64)
525+
#elif CPU(ARM64)
526+
527+
void JIT::emit_op_mod(const JSInstruction* currentInstruction)
528+
{
529+
auto bytecode = currentInstruction->as<OpMod>();
530+
VirtualRegister result = bytecode.m_dst;
531+
VirtualRegister op1 = bytecode.m_lhs;
532+
VirtualRegister op2 = bytecode.m_rhs;
533+
534+
emitGetVirtualRegister(op1, jsRegT10);
535+
emitGetVirtualRegister(op2, jsRegT32);
536+
537+
addSlowCase(branchIfNotInt32(jsRegT10));
538+
addSlowCase(branchIfNotInt32(jsRegT32));
539+
540+
GPRReg dividendGPR = jsRegT10.payloadGPR();
541+
GPRReg divisorGPR = jsRegT32.payloadGPR();
542+
GPRReg quotientThenRemainderGPR = regT4;
543+
544+
addSlowCase(branchTest32(Zero, divisorGPR));
545+
546+
// This is doing: x - ((x / y) * y)
547+
div32(dividendGPR, divisorGPR, quotientThenRemainderGPR);
548+
// This should only overflow for INT32_MIN % -1 but that will end up with quotientThenRemainderGPR == 0 and finally yield -0.0 as expected.
549+
multiplySub32(quotientThenRemainderGPR, divisorGPR, dividendGPR, quotientThenRemainderGPR);
550+
551+
// Make sure we're not accidentally producing a positive zero when it should be a negative zero.
552+
Jump numeratorPositive = branch32(GreaterThanOrEqual, dividendGPR, TrustedImm32(0));
553+
Jump nonZeroRemainder = branchTest32(NonZero, quotientThenRemainderGPR);
554+
moveValue(jsDoubleNumber(-0.0), jsRegT10);
555+
Jump done = jump();
556+
557+
numeratorPositive.link(this);
558+
nonZeroRemainder.link(this);
559+
560+
boxInt32(quotientThenRemainderGPR, jsRegT10);
561+
done.link(this);
562+
563+
emitPutVirtualRegister(result, jsRegT10);
564+
}
565+
566+
void JIT::emitSlow_op_mod(const JSInstruction*, Vector<SlowCaseEntry>::iterator& iter)
567+
{
568+
linkAllSlowCases(iter);
569+
570+
JITSlowPathCall slowPathCall(this, slow_path_mod);
571+
slowPathCall.call();
572+
}
573+
574+
#else // CPU(X86_64) || CPU(ARM64)
526575

527576
void JIT::emit_op_mod(const JSInstruction*)
528577
{
@@ -535,7 +584,7 @@ void JIT::emitSlow_op_mod(const JSInstruction*, Vector<SlowCaseEntry>::iterator&
535584
UNREACHABLE_FOR_PLATFORM();
536585
}
537586

538-
#endif // CPU(X86_64)
587+
#endif // CPU(X86_64) || CPU(ARM64)
539588

540589
/* ------------------------------ END: OP_MOD ------------------------------ */
541590

‎Source/JavaScriptCore/lol/LOLJIT.cpp‎

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,6 @@ void LOLJIT::privateCompileSlowCases()
668668
DEFINE_SLOWCASE_OP(op_loop_hint)
669669
DEFINE_SLOWCASE_OP(op_enter)
670670
DEFINE_SLOWCASE_OP(op_check_traps)
671-
DEFINE_SLOWCASE_OP(op_mod)
672671
DEFINE_SLOWCASE_OP(op_pow)
673672
DEFINE_SLOWCASE_OP(op_mul)
674673
DEFINE_SLOWCASE_OP(op_negate)
@@ -699,6 +698,7 @@ void LOLJIT::privateCompileSlowCases()
699698
DEFINE_SLOWCASE_SLOW_OP(rshift, OpRshift)
700699
DEFINE_SLOWCASE_SLOW_OP(urshift, OpUrshift)
701700
DEFINE_SLOWCASE_SLOW_OP(div, OpDiv)
701+
DEFINE_SLOWCASE_SLOW_OP(mod, OpMod)
702702
DEFINE_SLOWCASE_SLOW_OP(create_this, OpCreateThis)
703703
DEFINE_SLOWCASE_SLOW_OP(create_promise, OpCreatePromise)
704704
DEFINE_SLOWCASE_SLOW_OP(create_generator, OpCreateGenerator)
@@ -2871,6 +2871,90 @@ void LOLJIT::emitSlow_op_sub(const JSInstruction* currentInstruction, Vector<Slo
28712871
emitMathICSlow<OpSub>(subIC, currentInstruction, operationValueSubProfiledOptimize, operationValueSubProfiled, operationValueSubOptimize, iter);
28722872
}
28732873

2874+
#if CPU(X86_64)
2875+
2876+
void LOLJIT::emit_op_mod(const JSInstruction* currentInstruction)
2877+
{
2878+
auto bytecode = currentInstruction->as<OpMod>();
2879+
auto allocations = m_fastAllocator.allocate(*this, bytecode, m_bytecodeIndex);
2880+
auto [ lhsRegs, rhsRegs ] = allocations.uses;
2881+
auto [ dstRegs ] = allocations.defs;
2882+
2883+
// Make sure registers are correct for x86 IDIV instructions.
2884+
ASSERT(regT0 == X86Registers::eax);
2885+
auto eax = X86Registers::eax;
2886+
auto edx = X86Registers::edx;
2887+
auto ecx = X86Registers::ecx;
2888+
2889+
addSlowCase(branchIfNotInt32(lhsRegs));
2890+
addSlowCase(branchIfNotInt32(rhsRegs));
2891+
addSlowCase(branchTest32(Zero, rhsRegs.payloadGPR()));
2892+
2893+
// Check for INT32_MIN % -1 (would overflow on IDIV)
2894+
Jump denominatorNotNeg1 = branch32(NotEqual, rhsRegs.payloadGPR(), TrustedImm32(-1));
2895+
addSlowCase(branch32(Equal, lhsRegs.payloadGPR(), TrustedImm32(INT32_MIN)));
2896+
denominatorNotNeg1.link(this);
2897+
2898+
move(rhsRegs.payloadGPR(), ecx);
2899+
move(lhsRegs.payloadGPR(), eax);
2900+
2901+
// Sign extend eax to edx:eax
2902+
x86ConvertToDoubleWord32(eax, edx);
2903+
// Perform division: quotient in eax, remainder in edx
2904+
x86Div32(ecx);
2905+
2906+
// Check for negative zero result: if numerator was negative and remainder is 0
2907+
Jump numeratorPositive = branch32(GreaterThanOrEqual, lhsRegs.payloadGPR(), TrustedImm32(0));
2908+
addSlowCase(branchTest32(Zero, edx));
2909+
numeratorPositive.link(this);
2910+
2911+
// Box the remainder result
2912+
boxInt32(edx, dstRegs);
2913+
2914+
m_fastAllocator.releaseScratches(allocations);
2915+
}
2916+
2917+
#elif CPU(ARM64)
2918+
2919+
void LOLJIT::emit_op_mod(const JSInstruction* currentInstruction)
2920+
{
2921+
auto bytecode = currentInstruction->as<OpMod>();
2922+
auto allocations = m_fastAllocator.allocate(*this, bytecode, m_bytecodeIndex);
2923+
auto [ lhsRegs, rhsRegs ] = allocations.uses;
2924+
auto [ dstRegs ] = allocations.defs;
2925+
2926+
addSlowCase(branchIfNotInt32(lhsRegs));
2927+
addSlowCase(branchIfNotInt32(rhsRegs));
2928+
2929+
GPRReg dividendGPR = lhsRegs.payloadGPR();
2930+
GPRReg divisorGPR = rhsRegs.payloadGPR();
2931+
GPRReg quotientThenRemainderGPR = s_scratch;
2932+
// GPRReg multiplyAnswerGPR = s_scratch;
2933+
addSlowCase(branchTest32(Zero, divisorGPR));
2934+
2935+
// This is doing: x - ((x / y) * y)
2936+
div32(dividendGPR, divisorGPR, quotientThenRemainderGPR);
2937+
// This should only overflow for INT32_MIN % -1 but that will end up with quotientThenRemainderGPR == 0 and finally yield -0.0 as expected.
2938+
multiplySub32(quotientThenRemainderGPR, divisorGPR, dividendGPR, quotientThenRemainderGPR);
2939+
2940+
// Make sure we're not accidentally producing a positive zero when it should be a negative zero.
2941+
Jump numeratorPositive = branch32(GreaterThanOrEqual, dividendGPR, TrustedImm32(0));
2942+
Jump nonZeroRemainder = branchTest32(NonZero, quotientThenRemainderGPR);
2943+
moveValue(jsDoubleNumber(-0.0), dstRegs);
2944+
Jump done = jump();
2945+
2946+
numeratorPositive.link(this);
2947+
nonZeroRemainder.link(this);
2948+
2949+
boxInt32(quotientThenRemainderGPR, dstRegs);
2950+
done.link(this);
2951+
2952+
m_fastAllocator.releaseScratches(allocations);
2953+
}
2954+
#else
2955+
#error "Unsupported Architecture"
2956+
#endif
2957+
28742958
void LOLJIT::emit_op_negate(const JSInstruction* currentInstruction)
28752959
{
28762960
UnaryArithProfile* arithProfile = &m_unlinkedCodeBlock->unaryArithProfile(currentInstruction->as<OpNegate>().m_profileIndex);

‎Source/JavaScriptCore/lol/LOLJIT.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ namespace JSC::LOL {
5353
macro(op_add) \
5454
macro(op_mul) \
5555
macro(op_sub) \
56+
macro(op_mod) \
5657
macro(op_negate) \
5758
macro(op_inc) \
5859
macro(op_dec) \

‎Source/JavaScriptCore/lol/LOLRegisterAllocator.h‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,24 @@ auto RegisterAllocator<Backend>::allocate(Backend& jit, const OpDec& instruction
609609
return allocateImpl<0>(jit, instruction, index, uses, defs);
610610
}
611611

612+
template<typename Backend>
613+
auto RegisterAllocator<Backend>::allocate(Backend& jit, const OpMod& instruction, BytecodeIndex index)
614+
{
615+
std::array<AllocationHint, 2> uses = { instruction.m_lhs, instruction.m_rhs };
616+
std::array<AllocationHint, 1> defs = { instruction.m_dst };
617+
618+
auto result = allocateImpl<0>(jit, instruction, index, uses, defs);
619+
#if CPU(X86_64)
620+
// TODO: FIX X86 clobbering rules for eax/edx/ecx. This is inefficient and hacky.
621+
m_allocator.flushAllRegisters(*this);
622+
Location& dstLocation = locationOfImpl(instruction.m_dst);
623+
dstLocation.regs = result.defs[0];
624+
ASSERT(!dstLocation.isFlushed);
625+
m_allocator.bind(result.defs[0].payloadGPR(), instruction.m_dst, index.offset());
626+
#endif
627+
return result;
628+
}
629+
612630
template<typename Backend>
613631
auto RegisterAllocator<Backend>::allocate(Backend& jit, const OpToThis& instruction, BytecodeIndex index)
614632
{

0 commit comments

Comments
 (0)