Fix ARM64 layout issues #10071
Merged
Fix ARM64 layout issues #10071
Conversation
This file contains hidden or 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
* Change the definition of YGUndefined from NAN (contra the comment in
YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates
a quiet NaN. The problem with NAN is that corecrt_math.h defines it as:
Which *seems* fine! However, because we are compiling with /fp:precise
(see: microsoft#4122 for more details), that multiplication actually happens at
runtime. And, unfortunately, we're sometimes seeing MXCSR set to round
down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th
SSE instructions that happen to run here (specifically, cvtpd2ps), cause
NAN, and thus YGUndefined, to yield a value of 0! What happens is that
cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to
1#.INF in single-precision, but instead to FLT_MAX!!! And, of course,
FLT_MAX*0 is 0, not qNaN. Oops.
Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0,
rather than doing any math, so, even with /fp:precise + bad rounding
modes, YGUndefined is, in fact, a quiet NaN.
* Revert "Change the definition of YGUndefined from NAN (contra the comment in"
This reverts commit 3fb68a2.
Rather than trying to fork YGValue.h, we'll address the underlying
problem by not using /fp:strict for yoga.cpp
* Compiling yoga.cpp with /fp:strict causes
microsoft#8675. We use
this flag to address microsoft#4122, but in my manual testing, as well by
inspecting the disassembly that seemed to be responsible for issue 4122,
this workaround is no longer necessary.
* Patch yoga to use better definition for YGUndefined
* Change files
Co-authored-by: dannyvv <[email protected]>
Co-authored-by: Andrew Coates <[email protected]>
jonthysell
approved these changes
Jun 7, 2022
asklar
pushed a commit
to asklar/react-native-windows
that referenced
this pull request
Jun 18, 2022
* [0.64] Fix ARM64 layout issues in Office (microsoft#8689) * Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: microsoft#4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes microsoft#8675. We use this flag to address microsoft#4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <[email protected]> Co-authored-by: Andrew Coates <[email protected]> * Fix change file * react-native-platform-override upgrade Co-authored-by: Harold Pratt <[email protected]> Co-authored-by: dannyvv <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Port #8676 to main.
There was a hope to avoid this fork in main, with #8725, but that ended up hitting the original issue of why we turned on strict mode in the first place. I'd rather get a fix in so that we dont ship this issue again in upcoming releases. When we get a fix for the strict mode, we could remove this patch.
Microsoft Reviewers: Open in CodeFlow