Experiment with if constexpr - #1394
Conversation
|
What are your thoughts @JohanMabille @serge-sans-paille @DiamonDinoia ? |
|
Hi @AntoinePrv, Some suggestions for the discussion. I would implement it like this: namespace detail {
template <class T, class A> inline constexpr bool has_mm_add =
std::is_base_of_v<sse2, A> && std::is_arithmetic_v<T>;
template <class T, class A> inline constexpr bool has_mm256_add =
std::is_base_of_v<avx, A> && (std::is_floating_point_v<T> || std::is_base_of_v<avx2, A>);
template <class T, class A> inline constexpr bool has_mm512_add =
std::is_base_of_v<avx512f, A>
&& (std::is_floating_point_v<T> || sizeof(T) >= 4 || std::is_base_of_v<avx512bw, A>);
}
template <class T, class A>
XSIMD_INLINE batch<T, A> add(batch<T, A> lhs, batch<T, A> rhs) noexcept
{
constexpr auto recurse_add = [](auto l, auto r) { return kernel::add(l, r); };
if constexpr (detail::has_mm512_add<T, A>) return detail::mm512_add(lhs, rhs);
else if constexpr (detail::has_mm256_add<T, A>) return detail::mm256_add(lhs, rhs);
else if constexpr (detail::has_mm_add<T, A>) return detail::mm_add(lhs, rhs);
else if constexpr (std::is_base_of_v<avx512f, A> || std::is_base_of_v<avx, A>)
return detail::apply_on_halves(recurse_add, lhs, rhs);
else detail::unsupported<T, A>();
}The approach you proposed has a bug. It is easy to make the mistake:: batch<float, avx> takes the else of the inner if constexpr (std::is_integral_v && std::is_base_of_v<avx2, A>) and goes to apply_on_halves -> two _mm_add_ps instead of one _mm256_add_ps An alternative which I like (I am biased here (https://poet.readthedocs.io/en/latest/): I vibe coded the following idea: https://github.com/DiamonDinoia/xsimd/tree/proto/if-constexpr-dispatch I do not have a preference yet for what is best. I want to avoid churning for the sake of churning so I would like us to land on something that meaningfully improves maintainability/debugging. |
Some ideas experimenting with a fully
if constexprstructure by architecture family related to #1267.The interesting files to look at are the new ones, especially in
arithmetic(thev15is temporary).We can ignore the CI failure for now.
Some ideas that shaped this:
_mm...) in helper function signature as they can cause overloading issues, preferringbatch. Ideally providing similar helpers across architectures.addhere) have a single entry point (maybe two later withbatch_constant).kernel_fwdto make it easier to build shared utilities and break circular headers.requires_archneededWhat is nice:
batchAPI are error proof: they will fail to compile if the operation is not supported on the batch.addimplementation on x86, you immediately see that by going fromavxtoavx2you get native support on integers, which is something some user might want to take into account when selecting the arch they target.The part I really like is the split between the intrinsic wrappers (
mm_add,mm256_add, etc) and the dispatch.It feels like a correct abstraction because what you find inside these functions is very predictable.
With this, it starts looking even more closely to the "recipes" that @serge-sans-paille mentioned.
These overloads and some of these dispatch rules are getting close to being generated from data / recipe.
Possibly reuse that data to generate xsimd for Rust, C...
What is not so nice:
In this case some condition are actually repeated between
kernel::addandkernel::detail::mm512_addSome ADL gotchas with template, e.g. SVE
svptrue<T>utility being a template, we *do need a foward declaration to use it inaddon Neon even if is in a discardedif constexprbranch.More verbose, and more LOC, though this can be reduced by changing the formater from
To
Where should we go?
First this is still some large code changes. It would be mistake IMHO to start a migration if we do not have a reasonable hope of finishing it (risking a never-ending migration). This was the first function so it took some more time to set up (still not fully building), but this is not the hardest function.
I think we may first need to focus on merging overloads with the given structure (e.g. replacing all use of
std::enable_ifon functions to instead useif constexpr). Removing more C++14 constructs. Then, focus on providing forward declaration to provide more code reuse. Finally merge the remaining arch overloads.In parallel we can start a data-driven approach to generate safe intrinsic wrappers.