Skip to content

Commit

Permalink
[0.69] Stop using /fp:strict to build yoga.cpp (#10179)
Browse files Browse the repository at this point in the history
* 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
#4122, and I did
not find any issues anymore.

Unfortunately, while the exact issue tracked by #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>

* Update baseHash of CompactValue.h

* Make change type 'patch' instead of 'prerelease'

* Change files

* Revert "Update baseHash of CompactValue.h"

This reverts commit c11c602.

* One more time computing the baseHash...

Co-authored-by: Danny van Velzen <dannyvv@microsoft.com>
  • Loading branch information
htpiv and dannyvv committed Jun 23, 2022
1 parent a0c6fa1 commit 85a3a87
Show file tree
Hide file tree
Showing 4 changed files with 218 additions and 4 deletions.
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Stop using /fp:strict to build yoga.cpp",
"packageName": "react-native-windows",
"email": "hpratt@microsoft.com",
"dependentChangeType": "patch"
}
4 changes: 0 additions & 4 deletions vnext/ReactCommon/ReactCommon.vcxproj
Expand Up @@ -72,10 +72,6 @@
<DisableSpecificWarnings>4715;4251;4800;4804;4305;4722;%(DisableSpecificWarnings)</DisableSpecificWarnings>
<PreprocessToFile>false</PreprocessToFile>
<ForcedIncludeFiles>pch.h;%(ForcedIncludeFiles)</ForcedIncludeFiles>
<!--
Using Strict FloatingPointModel on x64 to workaround a compiler issue, See https://github.com/microsoft/react-native-windows/issues/4122
-->
<FloatingPointModel Condition="'$(Platform)' == 'x64'">Strict</FloatingPointModel>
</ClCompile>
<Link>
<IgnoreAllDefaultLibraries>false</IgnoreAllDefaultLibraries>
Expand Down
204 changes: 204 additions & 0 deletions 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 <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
7 changes: 7 additions & 0 deletions vnext/overrides.json
Expand Up @@ -74,6 +74,13 @@
"baseHash": "95a6fd14e80719f9796942ffe59dd850a56d544c",
"issue": 9791
},
{
"type": "patch",
"file": "ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/CompactValue.h",
"baseFile": "ReactCommon/yoga/yoga/CompactValue.h",
"baseHash": "4c22c7d0ecadeecb97a9694ffaa508442d973470",
"issue": 10145
},
{
"type": "patch",
"file": "ReactCommon/Yoga.cpp",
Expand Down

0 comments on commit 85a3a87

Please sign in to comment.