Skip to content

Add c10::irange to ExecuTorch #8554

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

Merged
merged 3 commits into from
Feb 19, 2025
Merged
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
14 changes: 10 additions & 4 deletions runtime/core/portable_type/c10/README.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
We added an extra c10 directory so that runtime/core/portable_type/c10
This directory contains header files from `c10` in PyTorch core that
need to be used in ExecuTorch core. They are copied here rather than
being found through the torch pip package to keep the core build
hermetic for embedded use cases. The headers should be exact copies
from PyTorch core; if they are out of sync, please send a PR!

We added an extra c10 directory so that `runtime/core/portable_type/c10`
can be the directory to put on your include path, rather than
runtime/core/portable_type, because using runtime/core/portable_type
`runtime/core/portable_type`, because using `runtime/core/portable_type`
would cause all headers in that directory to be includeable with
`#include <foo.h>`. In particular, that includes
runtime/core/portable_type/complex.h, which would shadow the C99
complex.h standard header.
`runtime/core/portable_type/complex.h`, which would shadow the C99
`complex.h` standard header.
1 change: 1 addition & 0 deletions runtime/core/portable_type/c10/c10/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def define_common_targets():
"util/TypeSafeSignMath.h",
"util/bit_cast.h",
"util/floating_point_utils.h",
"util/irange.h",
Copy link
Contributor

Choose a reason for hiding this comment

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

is irange.h not part of headers from pytorch? or we decided that is not worth sharing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a pytorch header, but it is used in executorch core. therefore it must be copied over just like everything else in runtime/core/portable_type/c10/c10 to keep the build hermetic for embedded users

],
exported_preprocessor_flags = [
# NOTE: If we define C10_EMBEDDED to prevent Half and
Expand Down
123 changes: 123 additions & 0 deletions runtime/core/portable_type/c10/c10/util/irange.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// Copyright 2004-present Facebook. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the copy right in line with rest of the codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Add BSD license header

Also, i have a task to add a lint rule: #8418

Copy link
Contributor Author

Choose a reason for hiding this comment

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


#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

If this file is copied over, can you add a comment here to the effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you add a comment

no. the file should be identical to the version in pytorch core.


#include <c10/util/TypeSafeSignMath.h>

#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

does it need <algorithm>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not relevant; this is a direct copy from pytorch core.

#include <cstddef>
#include <iterator>
#include <type_traits>

namespace c10 {

namespace detail {

template <
typename I,
bool one_sided = false,
std::enable_if_t<std::is_integral_v<I>, int> = 0>
struct integer_iterator {
using iterator_category = std::input_iterator_tag;
using value_type = I;
using difference_type = std::ptrdiff_t;
using pointer = I*;
using reference = I&;

explicit integer_iterator(I value) : value(value) {}

I operator*() const {
return value;
}

I const* operator->() const {
return &value;
}

integer_iterator& operator++() {
++value;
return *this;
}

integer_iterator operator++(int) {
const auto copy = *this;
++*this;
return copy;
}

bool operator==(const integer_iterator& other) const {
if constexpr (one_sided) {
// Range-for loops' end test is `begin != end`, not `begin <
// end`. To handle `c10::irange(n)` where n < 0 (which should be
// empty), we just make `begin != end` fail whenever `end` is
// negative.
return is_negative(other.value) || value == other.value;
} else {
return value == other.value;
}
// Suppress "warning: missing return statement at end of non-void function"
// which Nvidia's Robert Crovella confirms is an NVCC compiler error
// here https://stackoverflow.com/a/64561686/752843 on 2020-10-27
// `__builtin_unreachable();` would be best here, but it's not
// available with all compilers. So we instead return an arbitrary
// value trusting that this line will, in fact, never be reached.
return false; // Horrible hack
}

bool operator!=(const integer_iterator& other) const {
return !(*this == other);
}

protected:
I value;
};

} // namespace detail

template <
typename I,
bool one_sided = false,
std::enable_if_t<std::is_integral_v<I>, bool> = true>
struct integer_range {
public:
integer_range(I begin, I end) : begin_(begin), end_(end) {}
using iterator = detail::integer_iterator<I, one_sided>;
iterator begin() const {
return begin_;
}
iterator end() const {
return end_;
}

private:
iterator begin_;
iterator end_;
};

/// Creates an integer range for the half-open interval [begin, end)
/// If end<=begin, then the range is empty.
/// The range has the type of the `end` integer; `begin` integer is
/// cast to this type.
template <
typename Integer1,
typename Integer2,
std::enable_if_t<std::is_integral_v<Integer1>, bool> = true,
std::enable_if_t<std::is_integral_v<Integer2>, bool> = true>
integer_range<Integer2> irange(Integer1 begin, Integer2 end) {
// If end<=begin then the range is empty; we can achieve this effect by
// choosing the larger of {begin, end} as the loop terminator
return {
static_cast<Integer2>(begin),
std::max(static_cast<Integer2>(begin), end)};
}

/// Creates an integer range for the half-open interval [0, end)
/// If end<=begin, then the range is empty
template <
typename Integer,
std::enable_if_t<std::is_integral_v<Integer>, bool> = true>
integer_range<Integer, true> irange(Integer end) {
return {Integer(), end};
}

} // namespace c10
3 changes: 3 additions & 0 deletions runtime/core/portable_type/targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ def define_common_targets():
"//executorch/runtime/core/exec_aten/...",
"//executorch/runtime/core/portable_type/test/...",
],
deps = [
"//executorch/runtime/core/portable_type/c10/c10:c10",
],
exported_deps = [
":scalar_type",
"//executorch/runtime/core:core",
Expand Down
4 changes: 3 additions & 1 deletion runtime/core/portable_type/tensor_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this diff per se, but makes me curious why we need in this file too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#include <cstdint>

#include <c10/util/irange.h>

#include <executorch/runtime/core/exec_aten/util/dim_order_util.h>
#include <executorch/runtime/core/exec_aten/util/scalar_type_util.h>
#include <executorch/runtime/core/exec_aten/util/tensor_shape_to_c_string.h>
Expand All @@ -30,7 +32,7 @@ ssize_t compute_numel(const TensorImpl::SizesType* sizes, ssize_t dim) {
dim == 0 || sizes != nullptr,
"Sizes must be provided for non-scalar tensors");
ssize_t numel = 1; // Zero-dimensional tensors (scalars) have numel == 1.
for (ssize_t i = 0; i < dim; ++i) {
for (const auto i : c10::irange(dim)) {
ET_CHECK_MSG(
sizes[i] >= 0,
"Size must be non-negative, got %d at dimension %zd",
Expand Down
Loading