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.

Sign up for GitHub

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

Jump to bottom

Fix compilation error and warning #26

Closed
wants to merge 1 commit into from
Closed

Fix compilation error and warning #26

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions experimental/bits/simd_builtin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2284,7 +2284,8 @@ template <typename _Abi>
const auto __absn = __vector_bitcast<_Ip>(_SuperImpl::_S_abs(__x));
const auto __maxn
= __vector_bitcast<_Ip>(__vector_broadcast<_Np>(__finite_max_v<_Tp>));
return __absn <= __maxn;
return _MaskImpl::template _S_convert<_Tp>(
_MaskImpl::_S_to_bits(__as_wrapper<_Np>(__absn <= __maxn)));
Comment on lines -2287 to +2288
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how the compiler is able to reach this code when _Abi is _VecBltnBtmsk. One of the if constexpr cases in _SimdImplX86::_S_isfinite should be true and therefore the call to _Base::_S_isfinite not instantiated.
In most cases the generic implementation in simd_builtin.h should not be expected to return bit masks. Also, how does any _VecBuiltin code still compile after this change?

Copy link
Author

@yaozhongxiao yaozhongxiao Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as issue #24 happened, I try to bypath the _SimdImplX86::_S_isfinite by call _Base::_S_isfinite directly that leads to the cases.
As far as I understand, the _Base::_S_isfinite works as the fallback case, it should be work as well, Is it?

After this change, other _VecBuiltin code still compiles successfully, because there are some implicit type castings during eval this expression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _SimdImplBuiltin functions sometimes work for bitmasks and sometimes don't. Since, at this point, only AVX512 (i.e. x86) uses the bitmask ABI, I prefer to keep bitmask code out of the generic implementation. IIRC SVE also uses bitmasks. So once we have a SVE we should revisit how to abstract it best. Having the _SimdImplBuiltin functions support both may become a maintenance burden, decrease optimization, and increase compile times.
I any case, let's figure out why #24 happens and if this is really the proper solution.

Copy link
Author

@yaozhongxiao yaozhongxiao Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
By the way, have you put SVE support on schedule?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still so much else to do that I can't start on SVE. I don't have a schedule for it, no. 😞

#endif
}

Expand Down Expand Up @@ -2342,11 +2343,13 @@ template <typename _Abi>
const auto __minn
= __vector_bitcast<_Ip>(__vector_broadcast<_Np>(__norm_min_v<_Tp>));
#if __FINITE_MATH_ONLY__
return __absn >= __minn;
return _MaskImpl::template _S_convert<_Tp>(
_MaskImpl::_S_to_bits(__as_wrapper<_Np>(__absn >= __minn)));
#else
const auto __maxn
= __vector_bitcast<_Ip>(__vector_broadcast<_Np>(__finite_max_v<_Tp>));
return __minn <= __absn && __absn <= __maxn;
return _MaskImpl::template _S_convert<_Tp>(_MaskImpl::_S_to_bits(
__as_wrapper<_Np>(__minn <= __absn && __absn <= __maxn)));
Comment on lines -2345 to +2352
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

#endif
}

Expand Down Expand Up @@ -2837,7 +2840,7 @@ template <typename _Abi>

// smart_reference access {{{2
template <typename _Tp, size_t _Np>
static constexpr void _S_set(_SimdWrapper<_Tp, _Np>& __k, int __i,
static constexpr void _S_set(_SimdWrapper<_Tp, _Np>& __k, size_t __i,
Comment on lines -2840 to +2843
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All _S_set functions declare the index parameter as int. To resolve the warning please change __i == __j to __i == int(__j). In any case, the warning is a false positive, since __j is a constant expression thus the situation Clang warns about cannot happen.

Copy link
Author

@yaozhongxiao yaozhongxiao Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think about that, but the index will be expected to be a negative value?
if not why not replace "int" with size_t?

furthermore, _S_set->_M_set, however, _M_set is used size_t for __i

    template <typename _Tp, size_t _Np, typename _Up>
      _GLIBCXX_SIMD_INTRINSIC constexpr static void
      _S_set(_SimdWrapper<_Tp, _Np>& __v, int __i, _Up&& __x) noexcept
      { __v._M_set(__i, static_cast<_Up&&>(__x)); }

void _M_set(size_t __i, _Tp __val) noexcept

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, _M_set using size_t while the rest uses int is inconsistent. Wrt. choosing the type: I could equally ask whether the index is expected to be a value greater than INT_MAX. Neither that, nor negative values are ever going to happen. The better question to ask: does this type model a mathematical integer where a + 1 > a or should it model a modulo ring where a + 1 typically is greater than a but sometimes may also be less than a. We don't need the latter. The former allows the compiler to reason better about the type and thus lead to better optimizations.
This is why the default integer type to use should always be signed. That the C++ standard library uses size_t for most indexes and size parameters is widely recognized as a mistake that we cannot fix nowadays.

bool __x) noexcept
{
if constexpr (is_same_v<_Tp, bool>)
Expand Down