Skip to content

elementwise_util: don't cast the result of compute_fun back to the common type #9385

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -643,13 +643,18 @@ target_link_options_shared_lib(executorch)
# Real integrations should supply their own YAML file that only lists the
# operators necessary for the models that will run.
#
if(EXECUTORCH_BUILD_KERNELS_OPTIMIZED)
# find pytorch lib here to make it available to all
# sub-directories. Find it before including portable so that
# optimized_portabale_kernels can use it.
find_package_torch_headers()
endif()

if(BUILD_EXECUTORCH_PORTABLE_OPS)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/kernels/portable)
endif()

if(EXECUTORCH_BUILD_KERNELS_OPTIMIZED)
# find pytorch lib here to make it available to all sub-directories
find_package_torch_headers()
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/kernels/optimized)
endif()

Expand Down
1 change: 1 addition & 0 deletions kernels/optimized/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ message("Generated files ${gen_command_sources}")
list(TRANSFORM _optimized_kernels__srcs PREPEND "${EXECUTORCH_ROOT}/")
add_library(optimized_kernels ${_optimized_kernels__srcs})
target_include_directories(optimized_kernels PRIVATE ${TORCH_INCLUDE_DIRS} "${EXECUTORCH_ROOT}/third-party/pocketfft")
target_compile_definitions(optimized_kernels PRIVATE ET_USE_PYTORCH_HEADERS)
target_link_libraries(
optimized_kernels PUBLIC executorch_core cpublas extension_threadpool
)
Expand Down
2 changes: 2 additions & 0 deletions kernels/portable/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ if(BUILD_OPTIMIZED_PORTABLE_KERNELS)
target_link_libraries(optimized_portable_kernels PRIVATE executorch)
target_link_libraries(optimized_portable_kernels PUBLIC extension_threadpool)
target_compile_options(optimized_portable_kernels PUBLIC ${_common_compile_options})
target_include_directories(optimized_portable_kernels PRIVATE ${TORCH_INCLUDE_DIRS})
target_compile_definitions(optimized_portable_kernels PRIVATE ET_USE_PYTORCH_HEADERS)
install(
TARGETS optimized_portable_kernels
DESTINATION lib
Expand Down
36 changes: 5 additions & 31 deletions kernels/portable/cpu/util/dtype_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ load_to_common_fn<CTYPE_COMMON> get_load_to_common_fn_bool_or_byte(
template <typename CTYPE_COMMON, const char* op_name>
load_to_common_fn<CTYPE_COMMON> get_load_to_common_fn_same_as_compute(
const Tensor& t) {
constexpr auto common_scalar_type = CppTypeToScalarType<CTYPE_COMMON>::value;
ET_CHECK_MSG(
t.scalar_type() == common_scalar_type,
"Unhandled dtype %s for %s",
::executorch::runtime::toString(common_scalar_type),
op_name);
return internal::load_and_convert<CTYPE_COMMON, CTYPE_COMMON>;
}

Expand Down Expand Up @@ -179,33 +173,13 @@ get_store_common_to_tensor_fn_bool_or_byte(const Tensor& t) {
template <typename CTYPE_COMMON, const char* op_name>
store_common_to_tensor_fn<CTYPE_COMMON>
get_store_common_to_tensor_fn_same_as_compute(const Tensor& t) {
constexpr auto common_scalar_type = CppTypeToScalarType<CTYPE_COMMON>::value;
ET_CHECK_MSG(
t.scalar_type() == common_scalar_type,
"Unhandled dtype %s for %s",
::executorch::runtime::toString(common_scalar_type),
op_name);
return internal::convert_and_store<CTYPE_COMMON, CTYPE_COMMON>;
// We already validate tensor types earlier in the process, so at
// this phase, treat same_as_compute the same as our widest
// SupportedTensorDtypes set.
return get_store_common_to_tensor_fn_realhbf16<CTYPE_COMMON, op_name>(t);
}

template <
typename CTYPE_COMMON,
const char* op_name,
std::enable_if_t<std::is_same_v<CTYPE_COMMON, float>, bool> = true>
store_common_to_tensor_fn<CTYPE_COMMON>
get_store_common_to_tensor_fn_same_as_common(const Tensor& t) {
void (*result)(CTYPE_COMMON, void*) = nullptr;
ET_SWITCH_THREE_TYPES(
Float, Half, BFloat16, t.scalar_type(), unused, op_name, CTYPE, [&]() {
result = internal::convert_and_store<CTYPE, CTYPE_COMMON>;
});
return result;
}

template <
typename CTYPE_COMMON,
const char* op_name,
std::enable_if_t<!std::is_same_v<CTYPE_COMMON, float>, bool> = true>
template <typename CTYPE_COMMON, const char* op_name>
store_common_to_tensor_fn<CTYPE_COMMON>
get_store_common_to_tensor_fn_same_as_common(const Tensor& t) {
return get_store_common_to_tensor_fn_same_as_compute<CTYPE_COMMON, op_name>(
Expand Down
23 changes: 19 additions & 4 deletions kernels/portable/cpu/util/elementwise_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ inline int64_t scalar_to<int64_t>(const Scalar& s) {
}

namespace internal {
template <typename Ignore, typename T>
using ignore_first_yield_second = T;

template <typename CTYPE_COMMON, typename Op, typename... Args>
using op_call_result =
std::invoke_result_t<Op, ignore_first_yield_second<Args, CTYPE_COMMON>...>;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need ignore_first_yield_second? why not use CTYPE_COMMON directly in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I need the ... to produce sizeof...(Args) instances of CTYPE_COMMON. If you have a suggestion for a better way to do that I would love to hear it; this is the best I could do.


template <
typename CTYPE_COMMON,
const char* op_name,
Expand Down Expand Up @@ -89,9 +96,16 @@ inline void apply_elementwise_fn(
inputs.first->element_size(),
})...};

const auto store_common_to_out =
internal::get_store_common_to_tensor_fn<CTYPE_COMMON, op_name>(
out, out_dtypes);
// NOTE: the result of compute_fun is not necessarily CTYPE_COMMON!
// For example, consider the possibility that compute_fun is a
// trigonometric function like acos, the common input type is bool,
// and the output type is float -- we would truncate acos(0) ~= 1.67
// to just 1. Conveniently, it costs us nothing at runtime to handle
// this correctly.
const auto store_compute_result_to_out =
internal::get_store_common_to_tensor_fn<
op_call_result<CTYPE_COMMON, Op, Args...>,
op_name>(out, out_dtypes);
char* const data_out = reinterpret_cast<char*>(out.mutable_data_ptr());
const auto out_element_size = out.element_size();

Expand All @@ -114,7 +128,8 @@ inline void apply_elementwise_fn(
.data_ptr[indexes[idx + 1] * input_info.element_size]);
}
auto result = std::apply(compute_fun, loaded_inputs);
store_common_to_out(result, &data_out[indexes[0] * out_element_size]);
store_compute_result_to_out(
result, &data_out[indexes[0] * out_element_size]);
}
});
}
Expand Down
3 changes: 2 additions & 1 deletion runtime/core/portable_type/c10/c10/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ def define_common_targets():
# -Wmacro-redefined, and we only care about getting
# reasonable vectorization and Sleef support.
"-DCPU_CAPABILITY_AVX2",
"-DET_USE_PYTORCH_HEADERS",
"-DHAVE_AVX2_CPU_DEFINITION",
"-DSTANDALONE_TORCH_HEADER",
] + get_sleef_preprocessor_flags(),
Expand All @@ -86,5 +87,5 @@ def define_common_targets():
# linker failure.
"ovr_config//cpu:arm64": get_sleef_preprocessor_flags(),
"DEFAULT": [],
}) + ["-DSTANDALONE_TORCH_HEADER"],
}) + ["-DET_USE_PYTORCH_HEADERS", "-DSTANDALONE_TORCH_HEADER"],
)
Loading