[ESIMD][NFC] Removed unused math APIs #3968
Conversation
Those APIs were never used since there is no way conditions `detail::is_esimd_vector<T2>::value` and `std::is_integral<T2>::value` would be both true at the same time.
| ESIMD_NODEBUG ESIMD_INLINE typename sycl::detail::enable_if_t< | ||
| detail::is_esimd_scalar<T1>::value && detail::is_esimd_vector<T2>::value && | ||
| std::is_integral<T0>::value && std::is_integral<T1>::value && | ||
| std::is_integral<T2>::value, |
There was a problem hiding this comment.
This looks like a bug. It should be std::is_integral<T2::element_type>::value
There was a problem hiding this comment.
I can't understand the value of this overload. Looks like it is an alias for the first version, where we have the first argument a vector, and the second argument a scalar. Here we just swap the order of arguments but to me, it really seems confusing. Unless I'm missing something.
There was a problem hiding this comment.
I think this was ported to match existing CM math function. IIRC there was customer request to add this convenience API so that when the parameters are specified in different order we will swap it transparently and it's still expected to work.
There was a problem hiding this comment.
Still looks confusing :)
esimd_lsr(scalar, vector) will actually do vector >> scalar. I don't see much value in this API. Readers of the code will probably get confused over the order of arguments.
There was a problem hiding this comment.
I see your confusion. This was added per previous customer request for programming convenience. I'm fine with removing it if it's confusing for ESIMD user. This could be done in kernel or utility file. I don't view it as a blocking issue.
| ESIMD_NODEBUG ESIMD_INLINE typename sycl::detail::enable_if_t< | ||
| detail::is_esimd_scalar<T1>::value && detail::is_esimd_vector<T2>::value && | ||
| std::is_integral<T0>::value && std::is_integral<T1>::value && | ||
| std::is_integral<T2>::value, |
|
@kbobrovs, please review. |
kbobrovs
left a comment
kbobrovs
left a comment
There was a problem hiding this comment.
does this change cause any backward compatibility issues? if not - LGTM
I think no. This API appears to be unusable, which means no one could potentially use it before. |
3 participants
Those APIs were never used since there is no way conditions
detail::is_esimd_vector<T2>::valueandstd::is_integral<T2>::valuewould be both true at the same time.