-
Notifications
You must be signed in to change notification settings - Fork 685
allow not memory planning mutable buffers #10071
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1640,13 +1640,26 @@ def placeholder( # noqa: C901 | |
else: | ||
spec.extra_tensor_info.fully_qualified_name = fqn | ||
spec.extra_tensor_info.location = TensorDataLocation.EXTERNAL | ||
if self.emitter_state.emit_mutable_buffer_names and is_mutable_buffer: | ||
if spec.extra_tensor_info is None: | ||
spec.extra_tensor_info = ExtraTensorInfo( | ||
fully_qualified_name=fqn, location=TensorDataLocation.SEGMENT | ||
|
||
if is_mutable_buffer: | ||
# Emit names if we are supposed to. | ||
if self.emitter_state.emit_mutable_buffer_names: | ||
if spec.extra_tensor_info is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little confused by this as i haven't kept track of the tensor info changes, what does it mean if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None just means no one has created one, if its not none they might not have populated the fqn so I populate it. I guess I could check what it is before overwriting it but the name is unique so it should always be safe to do this. extra_tensor_info is where we put optional info in the flatbuffer to not regress the size of every tensor by too much in embedded cases that dont need it. |
||
spec.extra_tensor_info = ExtraTensorInfo( | ||
fully_qualified_name=fqn, | ||
location=TensorDataLocation.SEGMENT, | ||
) | ||
else: | ||
spec.extra_tensor_info.fully_qualified_name = fqn | ||
# if We aren't emitting the name then it needs to be memory planned. | ||
elif spec.mem_id is None or spec.mem_offset is None: | ||
raise InternalError( | ||
self._emit_node_specific_error( | ||
self.node, | ||
# [2:] to remove the b_ prefix buffers get | ||
f'Mutable buffer "{target[2:]}" must have a memory id and offset if we are emitting it without a name. Please either memory plan your mutable buffers or call to_executorch with config=ExecutorchBackendConfig(emit_mutable_buffer_names=True)', | ||
) | ||
) | ||
else: | ||
spec.extra_tensor_info.fully_qualified_name = fqn | ||
|
||
# From the fqn find the corresponding tensor | ||
real_tensor = None | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1838,8 +1838,40 @@ def forward(self, x): | |
ep = to_edge(ep) | ||
# Lower the graph to executorch. | ||
ep = ep.to_executorch( | ||
config=ExecutorchBackendConfig(emit_mutable_buffer_names=True) | ||
config=ExecutorchBackendConfig( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe also add a post_init in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered this but since the MemoryPlanningPass is user configureable there is no guarantee memory_planning_pass.alloc_mutable_buffers exists. So I just check the end result in the emitter. |
||
emit_mutable_buffer_names=True, | ||
memory_planning_pass=MemoryPlanningPass(alloc_mutable_buffers=False), | ||
) | ||
) | ||
for val in ep.executorch_program.execution_plan[0].values: | ||
if isinstance(val, Tensor) and val.extra_tensor_info: | ||
self.assertEqual(val.extra_tensor_info.fully_qualified_name, "buffer") | ||
self.assertEqual(val.allocation_info, None) | ||
|
||
def test_emit_mutable_buffer_names_fails(self) -> None: | ||
class Net(nn.Module): | ||
def __init__(self): | ||
super().__init__() | ||
self.linear = nn.Linear(2, 2) | ||
self.register_buffer("buffer", torch.zeros(1, 2)) | ||
|
||
def forward(self, x): | ||
self.buffer.add_(1) | ||
return self.linear(x) + self.buffer | ||
|
||
net = Net() | ||
|
||
ep = export(net, (torch.randn(1, 2),), strict=True) | ||
# Lower the graph to edge dialect. | ||
ep = to_edge(ep) | ||
# Lower the graph to executorch. | ||
# Must emit mutable buffer names if we don't allocate mutable buffers | ||
with self.assertRaises(InternalError): | ||
ep.to_executorch( | ||
config=ExecutorchBackendConfig( | ||
emit_mutable_buffer_names=False, | ||
memory_planning_pass=MemoryPlanningPass( | ||
alloc_mutable_buffers=False | ||
), | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual core logic change, the rest of the changes are mostly piping a flag around