Fix compilation error and warning

[yaozhongxiao]

fix the building error from _SimdImplBuiltin::_S_isfinite
and building warning from _MaskImplBuiltin::_S_set

Test Plan: ninja/make build_tests

[yaozhongxiao]

solving issue: #25

[mattkretz]

Oh I'm sorry, I'll look into this PR next thing tomorrow.

[mattkretz]

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.

[yaozhongxiao]

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

[mattkretz]

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.

[mattkretz]

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?

[yaozhongxiao]

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.

[mattkretz]

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.

[yaozhongxiao]

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

[mattkretz]

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

[mattkretz]

As above.

[yaozhongxiao]

Thank you for your kind reply and comments, the remaining issue is the warning in _S_set.
but I think it's trivial, I will close this PR, do you agree or any suggestions?

static constexpr void _S_set(_SimdWrapper<_Tp, _Np>& __k, size_t __i,

[mattkretz]

Thank you for your kind reply and comments, the remaining issue is the warning in _S_set.
but I think it's trivial, I will close this PR, do you agree or any suggestions?

I agree with closing it. The int - size_t mismatch is not a bug and the warning doesn't show for anyone who includes it from the libstdc++ installation.