Conversation
a433576 to
e5b5b61
Compare
jfalcou
left a comment
There was a problem hiding this comment.
This looks like it go in the right direction.
A few things before this goes in.
There were actually three definitions of "native", not two - native_abi in arch/cpu/tags.hpp, has_native_abi_v in detail/has_abi.hpp and _::is_native_v in detail/is_native.hpp - and they disagreed on bundle_ as much as on bare tags. That's worth spelling out in the description, because unifying on the tags.hpp semantics flips has_native_abi_v from true to false for bundle types. Five call sites change meaning as a result: detail/category.hpp, kernel_bessel_i0/i1.hpp, impl/convert.hpp, and logical_not.hpp where it is a hard requires, so a bundle would no longer find an overload there. Nothing breaks today - the api/tuple tests build either way - but it is a deliberate semantic choice and it should be stated and pinned by a test rather than left implicit.
Which is the main thing missing: test/unit/meta/abi_traits.cpp is the old has_abi.cpp with the names swapped, so it never asserts the case this PR exists to fix. Please add the tag-level checks:
TTS_EXPECT_NOT( eve::native_abi<eve::emulated_> );
TTS_EXPECT_NOT( eve::native_abi<eve::aggregated_> );
TTS_EXPECT_NOT( eve::native_abi<eve::bundle_> );emulated_abi, bundle_abi, non_native_abi and regular_abi are not covered at all either from what I can see.
On your question about the helper variables and aliases: they are dead. Twelve of the thirteen has_*_abi_v / has_*_abi_t have zero uses across include/ and test/ once the call sites moved to the concepts. Only has_aggregated_component_v is still used. Drop the twelve and keep that one.
Two smaller things. eve::abi<T,N> now only exists under EVE_DOXYGEN_INVOKED, but the Doxygen block above it still documents the struct and its type member, so the published documentation will describe something user code cannot instantiate - either bring it back as an alias or reword the block around abi_t.
And there are a couple dozens trailing-whitespace lines in the two new headers that a clang-format pass will clear.
For what it is worth, could you run something like :
// Cross-check that PR #2374 does not change ABI selection nor native detection.
// Build against `main` and against the branch, then diff the two outputs.
#include <eve/wide.hpp>
#include <cstdio>
template<typename A> constexpr char const* tag_name()
{
if constexpr(std::is_same_v<A, eve::emulated_>) return "emulated_";
else if constexpr(std::is_same_v<A, eve::aggregated_>) return "aggregated_";
else if constexpr(std::is_same_v<A, eve::bundle_>) return "bundle_";
#if defined(SPY_SIMD_IS_X86)
else if constexpr(std::is_same_v<A, eve::x86_128_>) return "x86_128_";
else if constexpr(std::is_same_v<A, eve::x86_256_>) return "x86_256_";
else if constexpr(std::is_same_v<A, eve::x86_512_>) return "x86_512_";
#elif defined(SPY_SIMD_IS_ARM_SVE)
else if constexpr(std::is_same_v<A, eve::arm_sve_128_>) return "arm_sve_128_";
else if constexpr(std::is_same_v<A, eve::arm_sve_256_>) return "arm_sve_256_";
else if constexpr(std::is_same_v<A, eve::arm_sve_512_>) return "arm_sve_512_";
#elif defined(SPY_SIMD_IS_ARM)
else if constexpr(std::is_same_v<A, eve::arm_64_>) return "arm_64_";
else if constexpr(std::is_same_v<A, eve::arm_128_>) return "arm_128_";
#elif defined(SPY_SIMD_IS_PPC)
else if constexpr(std::is_same_v<A, eve::ppc_>) return "ppc_";
#elif defined(SPY_SIMD_IS_RISCV)
else if constexpr(std::is_same_v<A, eve::riscv_>) return "riscv_";
#endif
else return "?";
}
template<typename T, int N> void cell(char const* t)
{
std::printf(" %-6s N=%-3d -> %s\n", t, N, tag_name<eve::abi_t<T, eve::fixed<N>>>());
}
template<typename T> void row(char const* t)
{
cell<T,1>(t); cell<T,2>(t); cell<T,4>(t); cell<T,8>(t);
cell<T,16>(t); cell<T,32>(t); cell<T,64>(t);
}
int main()
{
std::puts("== abi_t<T, fixed<N>> ==");
row<float>("f32"); row<double>("f64");
row<std::int8_t>("i8"); row<std::int16_t>("i16");
row<std::int32_t>("i32"); row<std::int64_t>("i64");
row<eve::logical<float>>("Lf32");
std::printf(" %-6s N=%-3d -> %s\n", "tuple", 4,
tag_name<eve::abi_t<kumi::tuple<float,int>, eve::fixed<4>>>());
// Names below exist on both main and the branch, so the same file builds on either.
std::puts("== native detection ==");
std::printf(" has_native_abi_v<emulated_> = %d\n", (int)eve::has_native_abi_v<eve::emulated_>);
std::printf(" has_native_abi_v<aggregated_> = %d\n", (int)eve::has_native_abi_v<eve::aggregated_>);
std::printf(" has_native_abi_v<bundle_> = %d\n", (int)eve::has_native_abi_v<eve::bundle_>);
std::printf(" native_abi<emulated_> = %d\n", (int)eve::native_abi<eve::emulated_>);
std::printf(" native_abi<aggregated_> = %d\n", (int)eve::native_abi<eve::aggregated_>);
std::printf(" native_abi<bundle_> = %d\n", (int)eve::native_abi<eve::bundle_>);
std::printf(" has_native_abi_v<wide<float>> = %d\n", (int)eve::has_native_abi_v<eve::wide<float>>);
std::printf(" has_native_abi_v<wide<tuple<f,i>>> = %d\n", (int)eve::has_native_abi_v<eve::wide<kumi::tuple<float,int>>>);
constexpr auto big = 2 * eve::wide<float>::size();
std::printf(" has_native_abi_v<wide<float,2*native>> = %d\n", (int)eve::has_native_abi_v<eve::wide<float, eve::fixed<big>>>);
}
and checks nothign diverge on w/e architecture we have ?
|
Going to investigate the tests and ensure nothing changes. I am still unsure if bundle should be considered |
7c9252f to
0e36a24
Compare
|
Running the code snippet provided against main and the last commit returns the following diff. The visible changes are mainly that :
Thus it seems to now properly differentiate every property that is of interest. As per the trailing spaces that exist, I think we should ignore them for this PR. Once every pending PR is merged we should just do a round of clang-format on EVE and force the use of it via precommit. As a lot of other parts of the code are not formatted either. Your call |
63aab56 to
a90edc9
Compare
5e6454d to
f124abd
Compare
- Test the different ABI concepts at least as well as the already tested ones - eve::abi, removed in favor of type alias, fix associated DOXYGEN - traits removed as deemed useless.
There were several issues with the old mechanism that stayed as is. - spy::simd_instruction_set was checked in some cases and other not for the detection when we know for a fact that if the current width is incompatible with it we can fail early. - spy::simd_instruction_set returns it's result in bits when we were counting in bytes. Homogeneizing it feels easier to read ex : width == 128 => x86_128 - We now consider the width of the type to chose it's register abi This is done after checking if the type is supported or not (emulated). - This will potentially break some test as it might not resolves to a 1:1 matching with previous detection but feels like the correct way.
- The current way to handle abi/tags and so on is actually pretty architecture specific. Instead of relying on consteval functions and have the compiler need to run it s constexpr interpreter we can directly match the properties we want. Using concepts and inline variables to route the abi detection in the correct sens. This makes it simpler to specialize per architecture than to have a monolithic function handling them all. - We should probably do the same for the rest of the internals (namely tags, register selection and so on). - There is a point that s quite unclear : why whould we want to export abi specific tags and so on to the rest of the library when the targeted architecture does not support them anyway ? Shouldn't they simply be accessible from architecture specific implems ?
a824e36 to
23bb7e3
Compare
|
This should be considered after moving to NTTPs (#2345) as it will be easier to rebase that way. |
There were abi related traits and concepts in several place in EVE and some where not coherent.
Namely is_native_v and has_native_abi_v were not detecting the same ABIs to be native. This PR aims at fixing it.
Not to sure about wether or not the inline helper variables and types aliases are usefull.