[JSC] FTL: call operationToInt32 on the slow path of the x86_64 double to int32 conversion - #625
[JSC] FTL: call operationToInt32 on the slow path of the x86_64 double to int32 conversion#625robobun wants to merge 1 commit into
Conversation
…e to int32 conversion
sensibleDoubleToInt32() emits the call on its slow path with
callWithoutSideEffects(), so B3 may run that call before the branch that guards
it. hoistLoopInvariantValues does so when the double is loop-invariant: the call
moves to the loop pre-header and runs with a double that the truncation accepts.
operationToInt32SensibleSlow is only defined for a double that the truncation
rejected. Since 320644@main it asserts that, ASSERT(exp >= 63) in
toInt32AfterFailedTruncation(), so a build with assertions on aborts as soon as
the FTL compiles a loop such as
for (let i = 0; i < n; ++i) int32Array[i & 7] = 1.5;
A release build runs the call and drops its result.
Call operationToInt32 there. It accepts every double, and the other FTL path
already calls it the same way. The DFG keeps operationToInt32SensibleSlow: its
slow path generator runs only after the truncation failed.
Preview Builds
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes FTL codegen on a path where B3's effect model determines correctness, and both touched paths are CODEOWNER-gated (@ WebKit/jsc-reviewers), a human look is still worthwhile.
What was reviewed
- Confirmed
operationToInt32has the same signature (UCPUStrictInt32 (double)) asoperationToInt32SensibleSlowand wrapsJSC::toInt32, which is defined for every double — safe to call withEffects::none()after a hoist. - Checked the sibling
doubleToInt32WithLimitspath already usesoperationToInt32viacallWithoutSideEffects, so this aligns both slow paths. - Verified the remaining
operationToInt32SensibleSlowcaller (DFGSpeculativeJIT) goes through a slow-path generator that runs only after the truncate check fails, so itsASSERT(exp >= 63)still holds. - Checked the new stress test against
JSTests/README.mdrules: throws on failure, usestestLoopCount, no logging, onenew Functionper case so each loop tiers up independently.
Extended reasoning...
Overview
The PR is a two-file change: a one-token functional edit in Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (swapping operationToInt32SensibleSlow → operationToInt32 on the x86_64 sensibleDoubleToInt32 slow path, plus a comment), and a new regression test at JSTests/stress/ftl-to-int32-loop-invariant-double.js. The root cause is well-argued: the slow-path CCall is emitted with Effects::none() via callWithoutSideEffects, so B3's hoistLoopInvariantValues may lift it above the fastResult64 == INT64_MIN guard, at which point toInt32AfterFailedTruncation's ASSERT(exp >= 63) (MathCommon.h:155) fires on ordinary doubles in debug builds. Swapping to the total function operationToInt32 (which just calls JSC::toInt32) makes the pure-call contract sound regardless of placement.
Security risks
None identified. This is a debug-assert fix; release builds already produced correct results because the hoisted call's value only feeds the slow-path Upsilon, which isn't selected when the fast branch wins. There's no new attack surface — the replacement callee is an existing, already-JIT-callable pure math operation with an identical ABI, and the added test is data-only JS with no I/O.
Level of scrutiny
Medium-high. The edit itself is tiny and mirrors the adjacent doubleToInt32WithLimits path, but it lives in FTL lowering where getting the effect/purity model wrong can silently miscompile hot code. The reasoning about B3 hoisting pure calls past control dependence is subtle enough that a JSC reviewer should confirm the chosen fix (total callee) over the alternatives the author lists (marking the call controlDependent, or dropping the assert). CODEOWNERS routes both /Source/JavaScriptCore and /JSTests to @ WebKit/jsc-reviewers, which by itself is reason not to auto-approve.
Other factors
The new stress test looks well-constructed: it uses testLoopCount, fails by throwing, avoids logging, and generates a fresh Function per case so each loop can tier into FTL independently. It covers both the bitwise-op path and the typed-array-store path, and includes slow-path inputs (2**63 + 2048, NaN, infinities) whose hoisted result is observed — a nice check that operationToInt32 returns the right value there. I could not verify the "< 200ms in all configurations" requirement from JSTests/README.md without a build; with ~32 new Function bodies each looping testLoopCount times, that's worth spot-checking with --report-execution-time before merge.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe FTL slow path now calls ChangesFTL int32 conversion
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The conversion change preserves correct behavior when optimization hoists the slow-path call, with stress coverage for affected values and stores. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Problem
for (let i = 0; i < n; ++i) int32Array[i & 7] = 1.5;. Release builds give the right value.sensibleDoubleToInt32(ftl/FTLLowerDFGToB3.cpp:25462) callsoperationToInt32SensibleSlowthroughcallWithoutSideEffects, so B3 treats the call as pure. When the double is loop-invariant,hoistLoopInvariantValuesmoves the call from its rarely taken block to the loop pre-header. There it runs with a double that the truncation accepts, for example 1.5.operationToInt32SensibleSlowonly handles a double that the truncation rejected. Since 320644@main (inmainthrough Upgrade to upstream WebKit ccdcb8a026 #614) it asserts that.Fix
operationToInt32. That operation accepts every double, so B3 can run it anywhere. The other FTL path,doubleToInt32WithLimits, already calls it the same way.cvttsd2siqon a path that only NaN, the infinities and|x| >= 2^63take.operationToInt32SensibleSlow. Its slow path generator runs only after the truncation failed, so the assertion holds there.JSTests/stress/ftl-to-int32-loop-invariant-double.js(new) aborts on thejscof theautobuild-cf1b36ecdebug build with theftl-eager-no-cjitoptions. It passes on thejscof this PR's preview build (autobuild-preview-pr-625-fe20753d, debug and ASAN) with the same options, with the defaults, and with--useConcurrentJIT=0. Alsoto-int32-sensible.js,to-int32-sensible2.jsandto-int32-out-of-int32-range-doubles.js. In Bun: Bump WebKit (oven-sh/WebKit#625 preview): stop assert-enabled builds aborting on a ToInt32 that B3 hoists out of a loop bun#42333 pins that preview build. Its test fails 7 of 8 cases on a debug build ofmainand passes with the pin.Background
cvttsd2siqtruncates the double to an int64, and the low 32 bits are the result. The instruction returnsINT64_MINfor NaN, the infinities and|x| >= 2^63. Only then does the code take the slow path call.callWithoutSideEffectsemits a B3CCallwithEffects::none(). B3 may delete such a value, or move it to any point where its inputs exist.hoistLoopInvariantValuesis the loop-invariant code motion of B3. A value with no effects, and with all inputs defined outside the loop, moves to the loop pre-header. Unless it iscontrolDependent, it moves even from a block that does not run on every iteration.Notes
Which code reaches it. The double has to be loop-invariant for B3 and not for the DFG. Otherwise the DFG folds the conversion, or hoists the whole
ValueToInt32node with its branch. Two families do that:int32Array[j] = 1.5,uint8Array[i & 7] = 255.5. The conversion is part ofPutByVal, so the DFG has no node to fold.zero = Math.imul(0, i)(alsoi - i,(i * 0) | 0), then(zero + 0.5) | 0,(zero ** 2) >> 0,~(zero ** 2).Release builds. The B3 dump of a release build (
dumpB3GraphAtEachPhase) shows theCCallin the pre-header afterhoistLoopInvariantValues, with the constant double as its argument. Its result only reaches thePhithrough theUpsilonof the slow block, which does not run.function hot(i) { const zero = i - i; return (zero + 5.5) | 0; }summed over 3e6 iterations gives 15000000 on a release build at this commit.What hides it.
--useB3HoistLoopInvariantValues=0,--useFTLJIT=0,--useDFGJIT=0.Upstream. WebKit/WebKit
mainhas the same code inMathCommon.h,FTLLowerDFGToB3.cppandB3HoistLoopInvariantValues.cpp, and the same option default, so a debug build of upstream JSC aborts the same way.Alternatives.
ASSERT. The helper returns 0 for every|x| < 2^52, so a pure operation would stay registered that gives a wrong ToInt32 for most doubles. Nothing reads that result today. The assertion is also right for the two callers that do check first (toInt32and the DFG).CCallcontrolDependent. That keeps the smaller helper and stops the hoist. It needs a new way to build aCCallValuewith custom effects inFTL::Output, for a path that almost never runs.The test. Every loop uses
testLoopCount, so only the eager configurations reach the FTL. Before the fix, all 11 fast path conversions and all 6 fast path typed array stores abort one by one under theftl-eager-no-cjitoptions. The slow path inputs (2 ** 63 + 2048, NaN, the infinities) do not abort. They check the value of a hoisted call whose result is used. The file passes on the unfixedjscwith--useFTLJIT=0, with--useJIT=0, and with--useB3HoistLoopInvariantValues=0. WithtestLoopCountreplaced by 10000 it passes on node v26.3.0.Run time.
JSTests/README.mdasks for less than 200 ms in every configuration. On the releasejscof the preview build (bun-webkit-linux-amd64), best of 3 runs: default 33 ms,no-llint20 ms,no-cjit-validate-phases49 ms,ftl-no-cjit-b3o018 ms,ftl-eager51 ms,ftl-eager-no-cjit124 ms,ftl-eager-no-cjit-b3o1117 ms,dfg-eager31 ms,no-ftl28 ms,--useJIT=08 ms.