From 0d221b383c1e37ff97670e346efe7e5c4dfa2eff Mon Sep 17 00:00:00 2001 From: Harold Pratt <38818465+htpiv@users.noreply.github.com> Date: Mon, 20 Jun 2022 14:39:47 -0700 Subject: [PATCH] Stop using /fp:strict to build yoga.cpp (#8725) * Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing https://github.com/microsoft/react-native-windows/issues/4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by https://github.com/microsoft/react-native-windows/issues/4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen --- ...-0eabcbbe-3124-42fe-b30b-356332950c73.json | 7 + vnext/ReactCommon/ReactCommon.vcxproj | 4 - .../yoga/yoga/CompactValue.h | 204 ++++++++++++++++++ .../yoga/yoga/YGValue.h | 94 -------- vnext/overrides.json | 7 +- 5 files changed, 215 insertions(+), 101 deletions(-) create mode 100644 change/react-native-windows-0eabcbbe-3124-42fe-b30b-356332950c73.json create mode 100644 vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h delete mode 100644 vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h diff --git a/change/react-native-windows-0eabcbbe-3124-42fe-b30b-356332950c73.json b/change/react-native-windows-0eabcbbe-3124-42fe-b30b-356332950c73.json new file mode 100644 index 00000000000..8b507da795f --- /dev/null +++ b/change/react-native-windows-0eabcbbe-3124-42fe-b30b-356332950c73.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ```", + "packageName": "react-native-windows", + "email": "hpratt@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/vnext/ReactCommon/ReactCommon.vcxproj b/vnext/ReactCommon/ReactCommon.vcxproj index bf492b40aeb..aec8268a118 100644 --- a/vnext/ReactCommon/ReactCommon.vcxproj +++ b/vnext/ReactCommon/ReactCommon.vcxproj @@ -72,10 +72,6 @@ 4715;4251;4800;4804;4305;4722;%(DisableSpecificWarnings) false pch.h;%(ForcedIncludeFiles) - - Strict false diff --git a/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h b/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h new file mode 100644 index 00000000000..79f84b41696 --- /dev/null +++ b/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h @@ -0,0 +1,204 @@ +/* + * 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. + */ + +#pragma once + +#ifdef __cplusplus + +#ifdef __cpp_lib_bit_cast +#include +#endif +#include "YGValue.h" +#include "YGMacros.h" +#include +#include +#include + +static_assert( + std::numeric_limits::is_iec559, + "facebook::yoga::detail::CompactValue only works with IEEE754 floats"); + +#ifdef YOGA_COMPACT_VALUE_TEST +#define VISIBLE_FOR_TESTING public: +#else +#define VISIBLE_FOR_TESTING private: +#endif + +namespace facebook { +namespace yoga { +namespace detail { + +// This class stores YGValue in 32 bits. +// - The value does not matter for Undefined and Auto. NaNs are used for their +// representation. +// - To differentiate between Point and Percent, one exponent bit is used. +// Supported the range [0x40, 0xbf] (0xbf is inclusive for point, but +// exclusive for percent). +// - Value ranges: +// points: 1.08420217e-19f to 36893485948395847680 +// 0x00000000 0x3fffffff +// percent: 1.08420217e-19f to 18446742974197923840 +// 0x40000000 0x7f7fffff +// - Zero is supported, negative zero is not +// - values outside of the representable range are clamped +class YOGA_EXPORT CompactValue { + friend constexpr bool operator==(CompactValue, CompactValue) noexcept; + + public: + static constexpr auto LOWER_BOUND = 1.08420217e-19f; + static constexpr auto UPPER_BOUND_POINT = 36893485948395847680.0f; + static constexpr auto UPPER_BOUND_PERCENT = 18446742974197923840.0f; + + template + static CompactValue of(float value) noexcept { + if (value == 0.0f || (value < LOWER_BOUND && value > -LOWER_BOUND)) { + constexpr auto zero = + Unit == YGUnitPercent ? ZERO_BITS_PERCENT : ZERO_BITS_POINT; + return {zero}; + } + + constexpr auto upperBound = Unit == YGUnitPercent ? UPPER_BOUND_PERCENT : UPPER_BOUND_POINT; + if (value > upperBound || value < -upperBound) { + value = copysignf(upperBound, value); + } + + uint32_t unitBit = Unit == YGUnitPercent ? PERCENT_BIT : 0; + auto data = asU32(value); + data -= BIAS; + data |= unitBit; + return {data}; + } + + template + static CompactValue ofMaybe(float value) noexcept { + return std::isnan(value) || std::isinf(value) ? ofUndefined() : of(value); + } + + static constexpr CompactValue ofZero() noexcept { + return CompactValue{ZERO_BITS_POINT}; + } + + static constexpr CompactValue ofUndefined() noexcept { + return CompactValue{}; + } + + static constexpr CompactValue ofAuto() noexcept { + return CompactValue{AUTO_BITS}; + } + + constexpr CompactValue() noexcept : repr_(0x7FC00000) {} + + CompactValue(const YGValue &x) noexcept : repr_(uint32_t{0}) { + switch (x.unit) { + case YGUnitUndefined: + *this = ofUndefined(); + break; + case YGUnitAuto: + *this = ofAuto(); + break; + case YGUnitPoint: + *this = of(x.value); + break; + case YGUnitPercent: + *this = of(x.value); + break; + } + } + + operator YGValue() const noexcept { + switch (repr_) { + case AUTO_BITS: + return YGValueAuto; + case ZERO_BITS_POINT: + return YGValue{0.0f, YGUnitPoint}; + case ZERO_BITS_PERCENT: + return YGValue{0.0f, YGUnitPercent}; + } + + if (std::isnan(asFloat(repr_))) { + return YGValueUndefined; + } + + auto data = repr_; + data &= ~PERCENT_BIT; + data += BIAS; + + return YGValue{asFloat(data), repr_ & 0x40000000 ? YGUnitPercent : YGUnitPoint}; + } + + bool isUndefined() const noexcept { + return (repr_ != AUTO_BITS && repr_ != ZERO_BITS_POINT && repr_ != ZERO_BITS_PERCENT && std::isnan(asFloat(repr_))); + } + + bool isAuto() const noexcept { + return repr_ == AUTO_BITS; + } + + private: + uint32_t repr_; + + static constexpr uint32_t BIAS = 0x20000000; + static constexpr uint32_t PERCENT_BIT = 0x40000000; + + // these are signaling NaNs with specific bit pattern as payload they will be + // silenced whenever going through an FPU operation on ARM + x86 + static constexpr uint32_t AUTO_BITS = 0x7faaaaaa; + static constexpr uint32_t ZERO_BITS_POINT = 0x7f8f0f0f; + static constexpr uint32_t ZERO_BITS_PERCENT = 0x7f80f0f0; + + constexpr CompactValue(uint32_t data) noexcept : repr_(data) {} + + VISIBLE_FOR_TESTING uint32_t repr() { + return repr_; + } + + static uint32_t asU32(float f) { +#ifdef __cpp_lib_bit_cast + return std::bit_cast(f); +#else + uint32_t u; + static_assert(sizeof(u) == sizeof(f)); + std::memcpy(&u, &f, sizeof(f)); + return u; +#endif + } + + static float asFloat(uint32_t u) { +#ifdef __cpp_lib_bit_cast + return std::bit_cast(data); +#else + float f; + static_assert(sizeof(f) == sizeof(u)); + std::memcpy(&f, &u, sizeof(u)); + return f; +#endif + } +}; + + +template <> +CompactValue CompactValue::of(float) noexcept = delete; +template <> +CompactValue CompactValue::of(float) noexcept = delete; +template <> +CompactValue CompactValue::ofMaybe(float) noexcept = delete; +template <> +CompactValue CompactValue::ofMaybe(float) noexcept = delete; + +constexpr bool operator==(CompactValue a, CompactValue b) noexcept { + return a.repr_ == b.repr_; +} + +constexpr bool operator!=(CompactValue a, CompactValue b) noexcept { + return !(a == b); +} + +} // namespace detail +} // namespace yoga +} // namespace facebook + +#endif diff --git a/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h b/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h deleted file mode 100644 index 1853ccfec71..00000000000 --- a/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h +++ /dev/null @@ -1,94 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include -#include "YGEnums.h" -#include "YGMacros.h" - -#if defined(_MSC_VER) && defined(__clang__) -#define COMPILING_WITH_CLANG_ON_WINDOWS -#endif -#if defined(COMPILING_WITH_CLANG_ON_WINDOWS) -#include -constexpr float YGUndefined = std::numeric_limits::quiet_NaN(); -#else -YG_EXTERN_C_BEGIN - -#if defined(_MSC_VER) -#define YGUndefined __builtin_nanf("0") -#else -#define YGUndefined NAN -#endif - -#endif - -typedef struct YGValue { - float value; - YGUnit unit; -} YGValue; - -YOGA_EXPORT extern const YGValue YGValueAuto; -YOGA_EXPORT extern const YGValue YGValueUndefined; -YOGA_EXPORT extern const YGValue YGValueZero; - -#if !defined(COMPILING_WITH_CLANG_ON_WINDOWS) -YG_EXTERN_C_END -#endif -#undef COMPILING_WITH_CLANG_ON_WINDOWS - -#ifdef __cplusplus - -inline bool operator==(const YGValue& lhs, const YGValue& rhs) { - if (lhs.unit != rhs.unit) { - return false; - } - - switch (lhs.unit) { - case YGUnitUndefined: - case YGUnitAuto: - return true; - case YGUnitPoint: - case YGUnitPercent: - return lhs.value == rhs.value; - } - - return false; -} - -inline bool operator!=(const YGValue& lhs, const YGValue& rhs) { - return !(lhs == rhs); -} - -inline YGValue operator-(const YGValue& value) { - return {-value.value, value.unit}; -} - -namespace facebook { -namespace yoga { -namespace literals { - -inline YGValue operator"" _pt(long double value) { - return YGValue{static_cast(value), YGUnitPoint}; -} -inline YGValue operator"" _pt(unsigned long long value) { - return operator"" _pt(static_cast(value)); -} - -inline YGValue operator"" _percent(long double value) { - return YGValue{static_cast(value), YGUnitPercent}; -} -inline YGValue operator"" _percent(unsigned long long value) { - return operator"" _percent(static_cast(value)); -} - -} // namespace literals -} // namespace yoga -} // namespace facebook - -#endif diff --git a/vnext/overrides.json b/vnext/overrides.json index 0c367535916..7076e268b7b 100644 --- a/vnext/overrides.json +++ b/vnext/overrides.json @@ -39,9 +39,10 @@ }, { "type": "patch", - "file": "ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h", - "baseFile": "ReactCommon/yoga/yoga/YGValue.h", - "baseHash": "1beec1c755f33ebbd4ab9ec5e138cd1a3cfba3ee" + "file": "ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h", + "baseFile": "ReactCommon/yoga/yoga/CompactValue.h", + "baseHash": "493638f3a2cdf828c46bbb523240588c0ddedec1", + "issue": 10145 }, { "type": "patch",