Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable fast floating point model and fast debug linking #11466

Merged
8 commits merged into from
Oct 11, 2021
Merged

Enable fast floating point model and fast debug linking #11466

8 commits merged into from
Oct 11, 2021
+71 −3

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Oct 8, 2021 •
edited
Loading

This commit enables /fp:fast. This doubles the performance of the Delta E
computation in #11095 for instance. Additionally it re-enables two options for
debug builds which are normally enabled by default by Visual Studio.

PR Checklist

  • I work here
  • Tests added/passed

Validation Steps Performed

  • No change in binary size
  • No obvious change in behavior

@lhecker lhecker requested a review from DHowett October 8, 2021 20:41
DHowett
DHowett approved these changes Oct 8, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

@DHowett
Copy link
Member

DHowett commented Oct 8, 2021

I believe we should ship this with 1.12 so dE doesn't regress launch perf as significantly 😄

(how much speed are we losing, would you say, on startup right now?)

@DHowett
Copy link
Member

DHowett commented Oct 8, 2021

@msftbot merge this in 5 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 8, 2021
@ghost
Copy link

ghost commented Oct 8, 2021

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Fri, 08 Oct 2021 21:42:18 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@Rosefield
Copy link
Contributor

curious if this has any effect on directional movement if you have a bunch of panes open. It should be fine since I'm checking for an epsilon of 1e-4, but who knows how bad accuracy could get.

@lhecker
Copy link
Member Author

lhecker commented Oct 8, 2021

The precise model isn't actually really that much more precise than the fast one...
It's precise because it handles all the corner cases (NaN stuff) while the fast one doesn't.
You can read more about it here: https://devblogs.microsoft.com/cppblog/do-you-prefer-fast-or-precise/

@DHowett
Copy link
Member

DHowett commented Oct 8, 2021

build's failing, curiously!

@lhecker
Copy link
Member Author

lhecker commented Oct 8, 2021

The tests work on my AMD CPU. The CI servers are clearly running on Intel CPUs. heh. 😎
In all seriousness however it a) sucks because I can't debug it and b) I'm surprised that TAEF doesn't compare floats using a sensible epsilon.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@lhecker
Copy link
Member Author

lhecker commented Oct 9, 2021

image

dab

ez

@lhecker
Copy link
Member Author

lhecker commented Oct 9, 2021

@msftbot merge this in 5 minutes

@ghost
Copy link

ghost commented Oct 9, 2021

Hello @lhecker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Sat, 09 Oct 2021 01:18:54 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett
DHowett requested changes Oct 9, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking to review the new!

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2021
DHowett
DHowett reviewed Oct 9, 2021
View reviewed changes
src/cascadia/ut_app/ColorHelperTests.cpp Outdated Show resolved Hide resolved
src/inc/consoletaeftemplates.hpp
}

const auto nDiff = static_cast<std::make_signed_t<U>>(*reinterpret_cast<U*>(&a) - *reinterpret_cast<U*>(&b));
const auto uDiff = static_cast<U>(nDiff < 0 ? -nDiff : nDiff);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the heck? This is legal??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And not UB?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is 100% undefined behavior. But it works because MSVC doesn't do strict aliasing. In C++20 you can use std::bit_cast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the question about legality of reading floats like that: Yeah that's legal, because the bit patterns of IEEE floats are clearly defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added til::bit_cast and replaced the reinterpret_cast hack.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2021
DHowett
DHowett reviewed Oct 11, 2021
View reviewed changes
src/inc/til/bit.h
#ifdef __cpp_lib_bit_cast
#warning "Replace til::bit_cast and __builtin_bit_cast with std::bit_cast"
#endif
return __builtin_bit_cast(To, _Val);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa

@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Oct 11, 2021
@DHowett DHowett added the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Oct 11, 2021
@DHowett
Copy link
Member

DHowett commented Oct 11, 2021

@lhecker the build is still failing, fyi. 😄

@DHowett DHowett removed zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Oct 11, 2021
@ghost ghost merged commit b036cab into main Oct 11, 2021
@ghost ghost deleted the dev/lhecker/compiler-options branch October 11, 2021 21:02
PankajBhojwani pushed a commit that referenced this pull request Oct 13, 2021
@lhecker @PankajBhojwani
Enable fast floating point model and fast debug linking (#11466)
385c6ee 
This commit enables /fp:fast. This doubles the performance of the Delta E
computation in #11095 for instance. Additionally it re-enables two options for
debug builds which are normally enabled by default by Visual Studio.

## PR Checklist
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* No change in binary size
* No obvious change in behavior
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lhecker @DHowett @Rosefield @carlos-zamora