Skip to content

Commit

Permalink
Could round rather than truncate in compileClampDoubleToByte?
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=72054
rdar://113734964

Reviewed by Yusuke Suzuki.

Right now there's a difference between our non-optimizing code and our C++ code for ClampedUint8Arrays.
In the C++ code we correctly do a round to nearest (ties to even) but in the optimizing JITs we do a round to inifinity.
This patch fixes our optimizing code to round to nearest too.

* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::clampDoubleToByte):
(JSC::DFG::compileClampDoubleToByte):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):

Canonical link: https://commits.webkit.org/267100@main
  • Loading branch information
kmiller68 committed Aug 21, 2023
1 parent ce7e7da commit c55d067
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 7 deletions.
15 changes: 15 additions & 0 deletions JSTests/stress/uint8clamped-rounding-mode.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
let array = new Uint8ClampedArray(1);
function foo(value) {
array[0] = value;
}
noInline(foo);

for (let i = 0; i < 1e6; ++i) {
foo(2.5);
if (array[0] != 2)
throw new Error(i);

foo(3.5);
if (array[0] != 4)
throw new Error(i);
}
9 changes: 2 additions & 7 deletions Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3777,12 +3777,11 @@ void SpeculativeJIT::compileValueRep(Node* node)

static double clampDoubleToByte(double d)
{
d += 0.5;
if (!(d > 0))
d = 0;
else if (d > 255)
d = 255;
return d;
return std::nearbyint(d);
}

static void compileClampIntegerToByte(JITCompiler& jit, GPRReg result)
Expand All @@ -3801,16 +3800,12 @@ static void compileClampDoubleToByte(JITCompiler& jit, GPRReg result, FPRReg sou
{
// Unordered compare so we pick up NaN
static constexpr double byteMax = 255;
static constexpr double half = 0.5;
jit.moveZeroToDouble(scratch);
MacroAssembler::Jump tooSmall = jit.branchDouble(MacroAssembler::DoubleLessThanOrEqualOrUnordered, source, scratch);
jit.loadDouble(SpeculativeJIT::TrustedImmPtr(&byteMax), scratch);
MacroAssembler::Jump tooBig = jit.branchDouble(MacroAssembler::DoubleGreaterThanAndOrdered, source, scratch);

jit.loadDouble(SpeculativeJIT::TrustedImmPtr(&half), scratch);
// FIXME: This should probably just use a floating point round!
// https://bugs.webkit.org/show_bug.cgi?id=72054
jit.addDouble(source, scratch);
jit.roundTowardNearestIntDouble(source, scratch);
jit.truncateDoubleToInt32(scratch, result);
MacroAssembler::Jump truncatedInt = jit.jump();

Expand Down
9 changes: 9 additions & 0 deletions Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20019,6 +20019,15 @@ IGNORE_CLANG_WARNINGS_END
unsure(continuation), unsure(withinRange));

m_out.appendTo(withinRange, continuation);
if (isClamped) {
PatchpointValue* patchpoint = m_out.patchpoint(Double);
patchpoint->append(ConstrainedValue(doubleValue, B3::ValueRep::SomeRegister));
patchpoint->setGenerator([=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
jit.roundTowardNearestIntDouble(params[1].fpr(), params[0].fpr());
});
patchpoint->effects = Effects::none();
doubleValue = patchpoint;
}
intValues.append(m_out.anchor(m_out.doubleToInt(doubleValue)));
m_out.jump(continuation);

Expand Down

0 comments on commit c55d067

Please sign in to comment.