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.

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

xtensor triggers xsimd error on arm64: <bool, xsimd::neon64> usage of batch type with unsupported type #945

Closed
drew-parsons opened this issue Sep 27, 2023 · 8 comments · Fixed by #973

Comments

@drew-parsons
Copy link

drew-parsons commented Sep 27, 2023

Debian CI tests of xtensor 0.24.7 running against xsimd 10.0.0 (g++ 13.2.0) are finding an error on arm64 architecture.
The test log is https://ci.debian.net/data/autopkgtest/unstable/arm64/x/xtensor/38197207/log.gz (also here)

This is with XSIMD_ENABLE_XTL_COMPLEX (xtl 0.7.5).

An excerpt of the error is

127s [ 26%] Building CXX object CMakeFiles/test_xtensor_lib.dir/test_xoptional_assembly.cpp.o
127s /usr/bin/g++ -DXSIMD_ENABLE_XTL_COMPLEX -DXTENSOR_USE_XSIMD  -DXSIMD_ENABLE_XTL_COMPLEX=1 -march=native -std=c++14 -Wunused-parameter -Wextra -Wreorder -Wconversion -Wno-sign-conversion  -Wold-style-cast -Wunused-variable -ftemplate-backtrace-limit=0 -O3 -DNDEBUG -MD -MT CMakeFiles/test_xtensor_lib.dir/test_xoptional_assembly.cpp.o -MF CMakeFiles/test_xtensor_lib.dir/test_xoptional_assembly.cpp.o.d -o CMakeFiles/test_xtensor_lib.dir/test_xoptional_assembly.cpp.o -c /tmp/autopkgtest-lxc.1p570fd4/downtmp/autopkgtest_tmp/test_xoptional_assembly.cpp
131s In file included from /usr/include/xsimd/types/xsimd_batch.hpp:493,
131s                  from /usr/include/xsimd/xsimd.hpp:61,
131s                  from /usr/include/xtensor/xtensor_config.hpp:61,
131s                  from /usr/include/xtensor/xexception.hpp:24,
131s                  from /usr/include/xtensor/xstorage.hpp:21,
131s                  from /usr/include/xtensor/xbuffer_adaptor.hpp:21,
131s                  from /usr/include/xtensor/xarray.hpp:19,
131s                  from /tmp/autopkgtest-lxc.1p570fd4/downtmp/autopkgtest_tmp/test_xoperation.cpp:13:
131s /usr/include/xsimd/types/xsimd_traits.hpp: In instantiation of ‘struct xsimd::detail::static_check_supported_config_emitter<bool, xsimd::neon64>’:
131s /usr/include/xsimd/types/xsimd_traits.hpp:84:19:   required from ‘void xsimd::detail::static_check_supported_config() [with T = bool; A = xsimd::neon64]’
131s /usr/include/xsimd/types/xsimd_api.hpp:437:55:   required from ‘xsimd::simd_return_type<From, To, A> xsimd::broadcast_as(From) [with To = int; A = neon64; From = bool; simd_return_type<From, To, A> = batch_bool<int, neon64>]’
131s /usr/include/xtensor/xscalar.hpp:952:53:   required from ‘xt_simd::simd_return_type<typename xt::xcontainer_inner_types<xt::xscalar<CT> >::value_type, requested_type> xt::xscalar<CT>::load_simd(size_type) const [with align = xt::inner_aligned_mode; requested_type = int; long unsigned int N = 4; CT = bool; xt_simd::simd_return_type<typename xt::xcontainer_inner_types<xt::xscalar<CT> >::value_type, requested_type> = xsimd::batch_bool<int, xsimd::neon64>; typename xt::xcontainer_inner_types<xt::xscalar<CT> >::value_type = bool; size_type = long unsigned int]’
...
131s /usr/include/xtensor/xarray.hpp:510:30:   required from ‘xt::xarray_container<EC, L, SC, Tag>::xarray_container(const xt::xexpression<E>&) [with E = xt::xfunction<xt::detail::logical_and, const xt::xarray_container<xt::uvector<bool, xsimd::aligned_allocator<bool, 16> >, xt::layout_type::row_major, xt::svector<long unsigned int, 4, std::allocator<long unsigned int>, true>, xt::xtensor_expression_tag>&, xt::xscalar<bool> >; EC = xt::uvector<bool, xsimd::aligned_allocator<bool, 16> >; xt::layout_type L = xt::layout_type::row_major; SC = xt::svector<long unsigned int, 4, std::allocator<long unsigned int>, true>; Tag = xt::xtensor_expression_tag]’
131s /tmp/autopkgtest-lxc.1p570fd4/downtmp/autopkgtest_tmp/test_xoperation.cpp:378:28:   required from ‘void xt::DOCTEST_ANON_SUITE_131::DOCTEST_ANON_TMP_150() [with TypeParam = xt::xarray_container<xt::uvector<double, xsimd::aligned_allocator<double, 16> >, xt::layout_type::row_major, xt::svector<long unsigned int, 4, std::allocator<long unsigned int>, true>, xt::xtensor_expression_tag>]’
131s /tmp/autopkgtest-lxc.1p570fd4/downtmp/autopkgtest_tmp/test_xoperation.cpp:372:9:   required from ‘xt::DOCTEST_ANON_SUITE_131::{anonymous}::DOCTEST_ANON_TMP_150ITERATOR<std::tuple<_El0, _El ...> >::DOCTEST_ANON_TMP_150ITERATOR(const char*, unsigned int, int) [with Type = xt::xarray_container<xt::uvector<double, xsimd::aligned_allocator<double, 16> >, xt::layout_type::row_major, xt::svector<long unsigned int, 4, std::allocator<long unsigned int>, true>, xt::xtensor_expression_tag>; Rest = {xt::xtensor_container<xt::uvector<double, xsimd::aligned_allocator<double, 16> >, 2, xt::layout_type::row_major, xt::xtensor_expression_tag>}]’
131s /tmp/autopkgtest-lxc.1p570fd4/downtmp/autopkgtest_tmp/test_xoperation.cpp:372:9:   required from here
131s /usr/include/xsimd/types/xsimd_traits.hpp:64:43: error: static assertion failed: usage of batch type with unsupported type
131s    64 |             static_assert(!A::supported() || xsimd::has_simd_register<T, A>::value,
131s       |                           ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
131s /usr/include/xsimd/types/xsimd_traits.hpp:64:43: note: ‘((! xsimd::neon64::supported()) || ((bool)std::integral_constant<bool, false>::value))’ evaluates to false

The error report seems to be saying xtensor is attempting to access an unsupported type. It's not clear to me if this should be considered a bug in xsimd or in xtensor. Or xtl.

@lamyj
Copy link
Contributor

lamyj commented Sep 27, 2023

A bit more additional info

#include <xtensor/xtensor.hpp>

int main()
{
    {
        xt::xtensor<double, 1> a{0., 1.};
        xt::xtensor<double, 1> b = a && 0.;
    }

    {
        xt::xtensor<bool, 1> a{false, true};
        //xt::xtensor<bool, 1> b = a && false;
    }

    return 0;
}

@drew-parsons
Copy link
Author

It's a bug in neon64 then :p

@serge-sans-paille
Copy link
Contributor

I confirm the following xsimd-only snippet works on x86-64 but not on aarch64

#include <xsimd/xsimd.hpp>

auto pain() {
  xsimd::batch<bool> x,y;
  return x && y;
}

I'll investigate, thanks for bringing that up!

@drew-parsons
Copy link
Author

In that case it's worth noting that xsimd is passing its own tests on arm64,
for v11 in particular, as 11.1.0-1exp4,
https://ci.debian.net/data/autopkgtest/unstable/arm64/x/xsimd/38216389/log.gz
(from https://ci.debian.net/packages/x/xsimd/unstable/arm64/ )

Does that mean xsimd::batch<bool> is not being tested in the xsimd unit tests?

@serge-sans-paille
Copy link
Contributor

@JohanMabille We currently don't have support for batch<bool>. We could alias to batch<uint8_t> or try to be smart, à la std::vector<bool>. The actual implementation would be generic for point-to-point operations, but rather complex for shuffles and the like. Likewise memory operations would be slightly more complex, so I advocate for an alias to batch<uint8_t>. What do you think?

@serge-sans-paille
Copy link
Contributor

After some extra digging: we actually don't really support xsimd::batch<bool, A> because we try to outsmart the user and decide it should be an xsimd::batch_bool<T, A>. It's all linked to simd_return_type and if I recall correctly, that's something that got baked in for xtensor. @JohanMabille, any thoughts?

@drew-parsons
Copy link
Author

armhf is also affected, apparently the same problem from neon, https://ci.debian.net/data/autopkgtest/unstable/armhf/x/xtensor/38875115/log.gz

@JohanMabille
Copy link
Member

JohanMabille commented Oct 12, 2023

It's all linked to simd_return_type and if I recall correctly, that's something that got baked in for xtensor

Not only for xtensor, it is required to promote types when playing with mixed arithmetic (std::complex<double> and double for instance) and to allow operations on "equivalent" types (like std::complex and xtl::complex for instance).

batch<bool> should definitely not support the same operations as batch<other_arithmetic_type>. I know the following is valid in C++:

bool b1 = true, b2 = false;
bool res = b1 + b2;

but I find it ugly and we should definitely avoid this in xsimd.

Also batch_bool must be constructible from boolscalars, not integers (even if C++ allows implicit conversions, we should not encourgae them in the API), so that's another reason for not aliasing it to batch<uint8_t>.

We could imagine aliasing it to batch_bool<uint8_t>, but then it would feel inconsistent with the return type of batch<double> == batch<double> for instance.

This issue is definitely a bug in the definition of simd_return_type, but we also need to understand why batch<bool> is allowed on x86/x64 architectures. and I don't remember why we allowed register for bool on x86/x64 architecture.

serge-sans-paille added a commit that referenced this issue Nov 12, 2023
It was only supported on Intel architectures and wasm, for no clear
reason. Provide a unified experience by enforcing batch_bool instead. If
someone really wants a batch of bool, then use another type to store it,
e.g. uint8_t.

Fix #945
serge-sans-paille added a commit that referenced this issue Nov 12, 2023
It was only supported on Intel architectures and wasm, for no clear
reason. Provide a unified experience by enforcing batch_bool instead. If
someone really wants a batch of bool, then use another type to store it,
e.g. uint8_t.

Fix #945
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants