mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.74k stars 439 forks source link

Add constexpr to Vector{2,3,4} and Vector<N, T> #597

Open sthalik opened 1 year ago

sthalik commented 1 year ago

Let's sprinkle constexpr with reckless abandon.

I'm still writing tests for all vector operators and subclasses. Would like your comments as to whether this is a reasonable approach.

mosra commented 1 year ago

Thank you for starting the work on this. I think having post-C++11 constexpr is desirable in general, but -- as we discussed on Gitter -- I still don't know how to resolve the major blocker here:

Let's say this would get merged, enabling constexpr for everything that can have it under C++14, including for example a 4x4 matrix multiplication, or a vector cross product, and people gradually start relying on these being constexpr. But then I'd want to add SIMD-optimized variants of those two (which I already have in a private branch), and I wouldn't be able to because in order to use SIMD intrinsics I'd have to drop the constexpr again, breaking everyone's code. Which I really don't want to. So how should we solve this? I see these options:

  1. Enable post-C++11 constexpr only for various mutable getters, and not for anything that is potentially SIMD-optimizable. Which probably goes against your use case, and it's also unclear where we should draw the line.
  2. Enable post-C++11 constexpr only on compilers that implement function "overloading" based on whether the function is constant-evaluated (i.e., that __builtin_constant_evaluated() you mentioned). Then, when I'd want to equip those with SIMD, I'd just add an "overload". What I'm not sure about is how those "overloads" are meant to be done, everything I found was about adding some a branch inside the implementation and deciding there. Is that the way? Or can I have some hypotetical int foo() and constexpr int foo() overloads?
  3. Or decide that SIMD will never be added directly to vector classes, and only used in other algorithms, and enable post-C++11 constexpr everywhere, as you do now. Sure, using SIMD in singular matrix/vector APIs is considered a "naive SIMD implementation" due to its relative inefficiency, nevertheless I'd like to keep that possibility.
  4. Other options / ideas?

So far I think option 2 would be the best. Let's say with a CORRADE_CONSTEXPR14_IF_CONSTEVAL macro that's non-empty only if the compiler supports C++14 constexpr and __builtin_constant_evaluated() or equivalent is available in the compiler in the currently chosen standard.

sthalik commented 1 year ago

Enable post-C++11 constexpr only on compilers that implement function "overloading" based on whether the function is constant-evaluated (i.e., that __builtin_constant_evaluated() you mentioned). Then, when I'd want to equip those with SIMD, I'd just add an "overload". What I'm not sure about is how those "overloads" are meant to be done, everything I found was about adding some a branch inside the implementation and deciding there. Is that the way?

A hypothetical CONSTEVAL macro could be defined as:

#if CORRADE_CXX_STANDARD >= 202002L
#    define CONSTEVAL (std::is_constant_evaluated())
#elif defined(__GNUG__) || defined(_MSC_VER) && _MSVC_LANG >= 202002L
#    define CONSTEVAL (__builtin_is_constant_evaluated())
#elif CORRADE_CXX_STANDARD  >= 201703L
#    define CONSTEVAL constexpr (Implementation::no_simd_v)
#else
#    define CONSTEVAL (false) /* or (true) */
#endif

Other options / ideas?

There are lots of out-of-class inline functions and conversion helpers. Which is why I attempted to put constexpr on everything in the first place.

Rewriting the vector code to be a single type and having less out-of-class inline functions lying around could be an option. Regarding not wanting it to become another Eigen, that shouldn't be the case unless it thinks it's a vector (including 3D cross product) and a matrix and a column vector (including 3D cross product) at the same time. I've had my own experience with cv::MatExpr as well so I'd rather not have it become like that as well.

I'll have to keep pushing to this branch in order to run it on your CI before it even compiles in the base non-constexpr case. Before that it's hard for me to answer as to what should be done about tests or which specific functions can be enabled at which compiler and standard conformance switch.

Or can I have some hypotetical int foo() and constexpr int foo() overloads?

Unfortunately not, and C++23 didn't fix it either.

codecov[bot] commented 1 year ago

Codecov Report

Base: 81.26% // Head: 81.26% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (beef203) compared to base (852fd16). Patch coverage: 100.00% of modified lines in pull request are covered.

:exclamation: Current head beef203 differs from pull request most recent head b7c5a6f. Consider uploading reports for the commit b7c5a6f to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #597 +/- ## ======================================= Coverage 81.26% 81.26% ======================================= Files 532 532 Lines 38965 38970 +5 ======================================= + Hits 31665 31670 +5 Misses 7300 7300 ``` | [Impacted Files](https://codecov.io/gh/mosra/magnum/pull/597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1) | Coverage Δ | | |---|---|---| | [src/Magnum/Math/TypeTraits.h](https://codecov.io/gh/mosra/magnum/pull/597/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL01hZ251bS9NYXRoL1R5cGVUcmFpdHMuaA==) | `100.00% <100.00%> (ø)` | | | [src/Magnum/Math/Vector.h](https://codecov.io/gh/mosra/magnum/pull/597/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL01hZ251bS9NYXRoL1ZlY3Rvci5o) | `100.00% <100.00%> (ø)` | | | [src/Magnum/Math/Vector2.h](https://codecov.io/gh/mosra/magnum/pull/597/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL01hZ251bS9NYXRoL1ZlY3RvcjIuaA==) | `100.00% <100.00%> (ø)` | | | [src/Magnum/Math/Vector3.h](https://codecov.io/gh/mosra/magnum/pull/597/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL01hZ251bS9NYXRoL1ZlY3RvcjMuaA==) | `100.00% <100.00%> (ø)` | | | [src/Magnum/Math/Vector4.h](https://codecov.io/gh/mosra/magnum/pull/597/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1#diff-c3JjL01hZ251bS9NYXRoL1ZlY3RvcjQuaA==) | `97.50% <100.00%> (+0.20%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vladim%C3%ADr+Vondru%C5%A1)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mosra commented 1 year ago

Sorry for abruptly cancelling the CIs -- can you merge (or rebase on) current master, in particular b9726a982305a59273b2eae8109ce748311d29fc? I'm trying to avoid unnecessary CircleCI builds to not run out of credits at the end of the month again :]

By all means, feel free to continue to abuse the CI for testing if stuff compiles, that's what it is for anyway, just merge that commit first. Thank you. It'll now run the essential Linux jobs first and only if they pass it'll continue further with other jobs and other platforms (such as shown here for current master build).

sthalik commented 1 year ago

It's pretty much ready in the current state and rebased onto master. But with some exceptions: