Skip to content

Commit

Permalink
Adjust MethodMeta's non-const indices by 1, and update all users (#315)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #315

First step in hiding the off-by-one non-const buffer indices from users. Since fewer users use `MethodMeta::num_non_const_buffers` today (relative to the version on Program), update the behavior here, along with all of its users.

After this, we can migrate the users of `Program::get_non_const_buffer_size` to this new API, fixing the off-by-one as we go.

I found all of these uses with the search

```
cd fbsource
xbgs -sl "non_const_buffer_size" | xargs grep -le '\<non_const_buffer_size\>'
```

Luckily, `Program`'s version of this is called `get_non_const_buffer_size` (with the extra `get_` prefix) so it was straightforward to find code that used `MethodMeta`'s methods.

Reviewed By: JacobSzwejbka

Differential Revision: D48762563

fbshipit-source-id: bd5b1b7dc7702cf0192261b535d59e8ba80b96e4
  • Loading branch information
dbort authored and facebook-github-bot committed Sep 13, 2023
1 parent 1f558c7 commit 5762802
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 12 deletions.
9 changes: 4 additions & 5 deletions exir/backend/test/demos/rpc/ExecutorBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,19 @@ class ExecutorBackend final : public PyTorchBackendInterface {
runtime_allocator, MemoryAllocator);
new (client_const_allocator) MemoryAllocator(0, nullptr);

auto num_buffers = method_meta->num_non_const_buffers();
size_t num_non_const_buffers = num_buffers - 1;
auto num_non_const_buffers = method_meta->num_non_const_buffers();

uint8_t** non_const_buffers = ET_ALLOCATE_LIST_OR_RETURN_ERROR(
runtime_allocator, uint8_t*, num_non_const_buffers);
MemoryAllocator* non_const_allocators = ET_ALLOCATE_LIST_OR_RETURN_ERROR(
runtime_allocator, MemoryAllocator, num_non_const_buffers);

for (size_t id = 1; id < num_buffers; ++id) {
for (size_t id = 0; id < num_non_const_buffers; ++id) {
auto buffer_size = method_meta->non_const_buffer_size(id);
uint8_t* buffer_i = ET_ALLOCATE_LIST_OR_RETURN_ERROR(
runtime_allocator, uint8_t, buffer_size.get());
non_const_buffers[id - 1] = buffer_i;
new (&non_const_allocators[id - 1])
non_const_buffers[id] = buffer_i;
new (&non_const_allocators[id])
MemoryAllocator(static_cast<uint32_t>(buffer_size.get()), buffer_i);
}

Expand Down
3 changes: 1 addition & 2 deletions extension/pybindings/pybindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ class Module final {
for (size_t i = 0; i < program_->num_methods(); ++i) {
auto name = program_->get_method_name(i).get();
auto method_meta = program_->method_meta(name).get();
// 1 on purpose because non-const are 1 indexed
for (size_t j = 1; j < method_meta.num_non_const_buffers(); j++) {
for (size_t j = 0; j < method_meta.num_non_const_buffers(); j++) {
int64_t buffer_size = method_meta.non_const_buffer_size(j).get();
if (non_const_buffer_sizes.find(j) == non_const_buffer_sizes.end()) {
non_const_buffer_sizes.insert({j, buffer_size});
Expand Down
9 changes: 7 additions & 2 deletions runtime/executor/method_meta.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ Result<TensorInfo> MethodMeta::output_tensor_meta(size_t index) const {
}

size_t MethodMeta::num_non_const_buffers() const {
return s_plan_->non_const_buffer_sizes()->size();
// Index zero is reserved internally, and we hide it from users. The actual
// number of buffers is one fewer than the actual size of this list in the
// program.
return s_plan_->non_const_buffer_sizes()->size() - 1;
}

Result<int64_t> MethodMeta::non_const_buffer_size(size_t index) const {
Expand All @@ -181,7 +184,9 @@ Result<int64_t> MethodMeta::non_const_buffer_size(size_t index) const {
"index %zu out of range. num_buffers: %zu",
index,
num_buffers);
return s_plan_->non_const_buffer_sizes()->Get(index);
// Index zero is reserved internally, and we hide it from users. Adjust the
// provided index to point to one of the actual buffers.
return s_plan_->non_const_buffer_sizes()->Get(index + 1);
}

} // namespace executor
Expand Down
6 changes: 3 additions & 3 deletions runtime/executor/test/method_meta_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,14 @@ TEST_F(MethodMetaTest, MethodMetaApi) {
EXPECT_EQ(method_meta->num_outputs(), 1);

// Appropriate amount of non_const_buffers
EXPECT_EQ(method_meta->num_non_const_buffers(), 2);
EXPECT_EQ(method_meta->num_non_const_buffers(), 1);

// Appropriate content of non_const_buffers
EXPECT_EQ(method_meta->non_const_buffer_size(1).get(), 48);
EXPECT_EQ(method_meta->non_const_buffer_size(0).get(), 48);

// Invalid index Errors
EXPECT_EQ(
method_meta->non_const_buffer_size(2).error(), Error::InvalidArgument);
method_meta->non_const_buffer_size(1).error(), Error::InvalidArgument);

EXPECT_EQ(
program_->method_meta("not_a_method").error(), Error::InvalidArgument);
Expand Down

0 comments on commit 5762802

Please sign in to comment.