forked from microsoft/react-native-windows
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Stop using /fp:strict to build yoga.cpp (microsoft#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 microsoft#4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by microsoft#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 <dannyvv@microsoft.com>
- Loading branch information
Showing
5 changed files
with
215 additions
and
101 deletions.
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/react-native-windows-0eabcbbe-3124-42fe-b30b-356332950c73.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
204 changes: 204 additions & 0 deletions
204
vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 <bit> | ||
#endif | ||
#include "YGValue.h" | ||
#include "YGMacros.h" | ||
#include <cmath> | ||
#include <cstdint> | ||
#include <limits> | ||
|
||
static_assert( | ||
std::numeric_limits<float>::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 <YGUnit Unit> | ||
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 <YGUnit Unit> | ||
static CompactValue ofMaybe(float value) noexcept { | ||
return std::isnan(value) || std::isinf(value) ? ofUndefined() : of<Unit>(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<YGUnitPoint>(x.value); | ||
break; | ||
case YGUnitPercent: | ||
*this = of<YGUnitPercent>(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<uint32_t>(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<float>(data); | ||
#else | ||
float f; | ||
static_assert(sizeof(f) == sizeof(u)); | ||
std::memcpy(&f, &u, sizeof(u)); | ||
return f; | ||
#endif | ||
} | ||
}; | ||
|
||
|
||
template <> | ||
CompactValue CompactValue::of<YGUnitUndefined>(float) noexcept = delete; | ||
template <> | ||
CompactValue CompactValue::of<YGUnitAuto>(float) noexcept = delete; | ||
template <> | ||
CompactValue CompactValue::ofMaybe<YGUnitUndefined>(float) noexcept = delete; | ||
template <> | ||
CompactValue CompactValue::ofMaybe<YGUnitAuto>(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 |
94 changes: 0 additions & 94 deletions
94
vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/YGValue.h
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters