Skip to content

[ESIMD][NFC] Removed unused math APIs #3968

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

Merged
kbobrovs merged 1 commit into from
Jun 26, 2021
Merged

[ESIMD][NFC] Removed unused math APIs #3968

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
20 changes: 0 additions & 20 deletions sycl/include/sycl/ext/intel/experimental/esimd/math.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,16 +303,6 @@ esimd_lsr(T1 src0, T2 src1, int flag = saturation_off) {
return Result[0];
}

template <typename T0, typename T1, typename T2>
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,
Copy link
Contributor

@kychendev kychendev Jun 22, 2021

Choose a reason for hiding this comment

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

This looks like a bug. It should be std::is_integral<T2::element_type>::value

Copy link
Contributor Author

@DenisBakhvalov DenisBakhvalov Jun 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kychendev kychendev Jun 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@DenisBakhvalov DenisBakhvalov Jun 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kychendev kychendev Jun 22, 2021

Choose a reason for hiding this comment

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

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.

decltype(esimd_lsr<T0>(T2(), T1()))>
esimd_lsr(T1 src0, T2 src1, int flag = saturation_off) {
return esimd_lsr<T0>(src1, src0, flag);
}

// esimd_asr
template <typename T0, typename T1, int SZ, typename U>
ESIMD_NODEBUG ESIMD_INLINE
Expand Down Expand Up @@ -346,16 +336,6 @@ esimd_asr(T1 src0, T2 src1, int flag = saturation_off) {
return Result[0];
}

template <typename T0, typename T1, typename T2>
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,
Copy link
Contributor

@kychendev kychendev Jun 22, 2021

Choose a reason for hiding this comment

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

Ditto

decltype(esimd_asr<T0>(T2(), T1()))>
esimd_asr(T1 src0, T2 src1, int flag = saturation_off) {
return esimd_asr<T0>(src1, src0, flag);
}

// esimd_imul
#ifndef ESIMD_HAS_LONG_LONG
// use mulh instruction for high half
Expand Down