Skip to content

Commit

Permalink
Avoid FMA contraction in makeDate
Browse files Browse the repository at this point in the history
Summary:
Newer versions of clang will automatically produce FMA instead of
separate multiply and add when the multiplication and addition are in
the same expression. This is undesirable because it results in
non-standard behaviour in makeDate, and breaks an existing test262
test.

We can do this in two ways:
1. We can split them into separate expressions
2. We can use `#pragma STDC FP_CONTRACT OFF`

Unfortunately, neither of these address the issue with GCC, which
uses `-ffp-contract=fast` by default, and cannot be overridden
except by using `volatile`.

To fix this, add `-ffp-contract=on` to our build flags, and split the
expressions.

Reviewed By: tmikov

Differential Revision: D51725454

fbshipit-source-id: 3d4c91bb32c619c05c53fd8b1962adbc862554aa
  • Loading branch information
neildhar authored and facebook-github-bot committed Dec 5, 2023
1 parent 60c8567 commit 957502e
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 1 deletion.
4 changes: 4 additions & 0 deletions cmake/modules/Hermes.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,10 @@ if (GCC_COMPATIBLE)
# released or XCode is fixed.
# add_flag_if_supported("-fno-strict-float-cast-overflow" NO_STRICT_FLOAT_CAST_OVERFLOW_FLAG)

# GCC uses fp-contract=fast by default, use "on" instead so that we can
# control it in source where needed.
add_flag_if_supported("-ffp-contract=on" FP_CONTRACT_FLAG)

# Disable range loop analysis warnings.
check_cxx_compiler_flag("-Wrange-loop-analysis" RANGE_ANALYSIS_FLAG)
append_if(RANGE_ANALYSIS_FLAG "-Wno-range-loop-analysis" CMAKE_CXX_FLAGS)
Expand Down
9 changes: 8 additions & 1 deletion lib/VM/JSLib/DateUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,14 @@ double makeDate(double day, double t) {
return std::numeric_limits<double>::quiet_NaN();
}

return day * MS_PER_DAY + t;
// Some compilers may contract the multiplication and addition into a single
// FMA when they are part of the same expression. This would result in
// non-standard results, so to avoid it, split them into separate expressions.
// Note that this applies only when compiling with -ffp-contract=on. If
// -ffp-contract=fast is used, the compiler will still be permitted to emit an
// FMA operation for the separate expressions.
double dayMs = day * MS_PER_DAY;
return dayMs + t;
}

//===----------------------------------------------------------------------===//
Expand Down
13 changes: 13 additions & 0 deletions test/hermes/date-fp-contract.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

// RUN: %hermes %s | %FileCheck --match-full-lines %s

// Check that the date implementation does not use fma which can produce
// non-compliant output.
print(Date.UTC(1970, 0, 213503982336, 0, 0, 0, -18446744073709552000))
// CHECK: 34447360
1 change: 1 addition & 0 deletions unittests/VMRuntime/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ set(RTSources
DateUtilTest.cpp
DecoratedObjectTest.cpp
DictPropertyMapTest.cpp
FPContractTest.cpp
GCBasicsTest.cpp
GCFinalizerTest.cpp
GCFragmentationTest.cpp
Expand Down
24 changes: 24 additions & 0 deletions unittests/VMRuntime/FPContractTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#include "gtest/gtest.h"

namespace {

/// Test that Hermes is compiled such that floating point multiplication and
/// addition in separate expressions cannot be combined into a single FMA
/// operation with higher intermediate precision. For instance, GCC uses
/// -ffp-contract=fast by default, which allows separate expressions to be
/// contracted. This is important because it is sometimes required by the JS
/// spec, for instance when computing dates.
TEST(FPContractTest, SeparateExpressionsFMA) {
double d0 = 213503982335.0 * 86400000.0;
double d1 = d0 - 18446744073709552000.0;
EXPECT_EQ(d1, 34447360.0);
}

} // namespace

0 comments on commit 957502e

Please sign in to comment.