[BE] Replace M_PI with c10::pi constexpr variable#50819
[BE] Replace M_PI with c10::pi constexpr variable#50819
Conversation
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
afd83b5 to
b5a4ef3
Compare
albanD
left a comment
There was a problem hiding this comment.
What is the original motivation for this change?
| namespace at { | ||
| namespace native { | ||
| //TODO: Replace me with std::numbers::pi when C++20 becomes available | ||
| template <typename T> constexpr T pi = 3.14159265358979323846L; |
There was a problem hiding this comment.
Do we know the impact of replacing M_PI with this value? Could that lead to some precision changes?
There was a problem hiding this comment.
To ensure there are none, I can add .cpp file that include <math.h> and adds static_assert(at:native::pi<double> == M_PI)
The motivation is not to pollute global namespace with M_PI define and also prevent build system brittleness on Windows, where including before <math.h> would result in M_PI not being defined
There was a problem hiding this comment.
That sounds good.
To ensure there are none, I can add .cpp file that include <math.h> and adds static_assert(at:native::pi == M_PI)
Would that be brittle on windows? haha
It would be nice though to have a dummy PR that adds this check and makes sure it passes on all our CI just as a sanity check.
3147319 to
b377ff9
Compare
albanD
left a comment
There was a problem hiding this comment.
compilation failures look related right?
Also, get rid of MSVC specific `_USE_MATH_DEFINES`
b377ff9 to
f77ff20
Compare
|
@albanD yes they are, which were mostly due to the fact, that there seem to be no good way of having a specialized constexpr variable (and BFloat16 constructor is not constexpr) But everything is green now. |
Codecov Report
@@ Coverage Diff @@
## master #50819 +/- ##
==========================================
- Coverage 81.02% 81.02% -0.01%
==========================================
Files 1916 1916
Lines 209343 209343
==========================================
- Hits 169624 169616 -8
- Misses 39719 39727 +8 |
albanD
left a comment
There was a problem hiding this comment.
LGTM thanks for the update
403cd48 to
84db488
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
While trying to access constexpr from kernel, NVCC complains that: ``` identifier "c10::pi< ::c10::Half> " is undefined in device code ```
walterddr
left a comment
There was a problem hiding this comment.
looks good. there are several more .cu/cc files in caffe2/operators that uses _USE_MATH_DEFINES for M_PI. Should we address them as well or leave it be?
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@walterddr good point, let me do it in separate PR |
| additional_summand = -c10::pi<scalar_t> / | ||
| compat_tan(c10::pi<scalar_t> * x); |
There was a problem hiding this comment.
FYI this was casting to accscalar_t
Summary: Also, get rid of MSVC specific `_USE_MATH_DEFINES` Test at compile time that c10::pi<double> == M_PI Pull Request resolved: pytorch#50819 Reviewed By: albanD Differential Revision: D25976330 Pulled By: malfet fbshipit-source-id: 8f3ddfd58a5aa4bd382da64ad6ecc679706d1284
Also, get rid of MSVC specific
_USE_MATH_DEFINESTest at compile time that c10::pi == M_PI