Skip to content

[serialize] Support torch.gather export via Circle GatherNd#790

Merged
mhs4670go merged 1 commit into
Samsung:mainfrom
mhs4670go:ga
Jun 22, 2026
Merged

[serialize] Support torch.gather export via Circle GatherNd#790
mhs4670go merged 1 commit into
Samsung:mainfrom
mhs4670go:ga

Conversation

@mhs4670go

Copy link
Copy Markdown
Contributor

This commit supports torch.gather export via Circle GatherNd.

TICO-DCO-1.0-Signed-off-by: seongwoo mhs4670go@naver.com

This commit supports torch.gather export via Circle GatherNd.

TICO-DCO-1.0-Signed-off-by: seongwoo <mhs4670go@naver.com>
@mhs4670go

Copy link
Copy Markdown
Contributor Author

@arkq

I agree with the overall lowering direction in #786: general torch.gather is not a good fit for Circle GATHER, and constructing full-coordinate indices for Circle GATHER_ND matches the expected semantics.

However, I am concerned about the PR's implementation strategy of keeping the original aten.gather node and changing its index input to a full-coordinate tensor, then relying on a metadata flag to make the serializer interpret it as GATHER_ND.

After this pass, the node no longer has valid aten.gather semantics. In other words, the graph still says aten.gather(input, dim, index), but index is now shaped like [..., rank] and is meant to be consumed by GATHER_ND. This makes the intermediate IR misleading and invalid from a PyTorch/ATen point of view. It also means later passes, meta propagation, debugging, or future optimizations may accidentally treat this node as a normal aten.gather, even though its actual meaning has already changed.

I think it would be cleaner and safer to materialize an explicit internal GatherNd-like representation instead:

aten.gather(input, dim, index)
  -> build full-coordinate indices
  -> internal_gather_nd(input, full_indices)
  -> serialize internal_gather_nd as Circle GATHER_ND

This would make the semantic boundary clear:

  • before the pass: the graph has PyTorch gather semantics
  • after the pass: the graph has explicit Circle/TFLite-style GATHER_ND semantics
  • the serializer no longer needs to infer a semantic change from a metadata flag

Because of this, I would prefer not to merge #786 in its current form. Instead, I suggest we keep the same high-level idea—lowering torch.gather to Circle GATHER_ND, which is why I posted this PR.

Could you review this PR instead?

@arkq arkq left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am concerned about the PR's implementation strategy of keeping the original aten.gather node and changing its index input to a full-coordinate tensor, then relying on a metadata flag to make the serializer interpret it as GATHER_ND.
After this pass, the node no longer has valid aten.gather semantics.

Yes, I totally agree with you. I did not know how to implement torch.gather to circle.gather_nd conversion while preserving torch graph validity - I was not aware that there is torch.ops.circle_custom.gather_nd. Your approach seems to be better indeed. The code looks OK.

@mhs4670go mhs4670go merged commit 4ceb6cb into Samsung:main Jun 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants