Skip to content
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

Add converter for index.Tensor_hacked_twin #98

Merged

Conversation

matthewfl
Copy link
Contributor

@matthewfl matthewfl commented Sep 18, 2024

  • Add converter for index.Tensor_hacked_twin -> tcp.gather & tcp.broadcast
  • Remove the tcp.custom_op variants of Tensor_hacked_twin
    • The Tcp variants do not have full coverage of the PyTorch, but we should seek to expand the coverage of our converters
  • Fix tcp.const when it is a dense resource of ints to cast the value
  • Add verifier for tcp.gather and tcp.const to ensure that is used correctly

@matthewfl matthewfl requested a review from zezhang September 18, 2024 19:04
Copy link
Contributor

@zezhang zezhang left a comment

Choose a reason for hiding this comment

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

LGTM! Can we also try to add it to the aot_compile test suite for e2e coverage?

%int1 = torch.constant.int 1
%int4 = torch.constant.int 4 // this is a dtype on arange....
%int-1 = torch.constant.int -1
%arange = torch.aten.arange.start_step %int0, %int1, %int1, %int4, %none, %none, %none : !torch.int, !torch.int, !torch.int, !torch.int, !torch.none, !torch.none, !torch.none -> !torch.vtensor<[1],si64>
Copy link
Contributor

Choose a reason for hiding this comment

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

torch.aten.arange.start_step will remain unchanged right? As we don't handle it in tcp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently this remains unchanged. I was thinking that we should add an optimization pass that removes this when it selects everything, but I have yet to do that

lib/Dialect/IR/TcpOps.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@navahgar navahgar left a comment

Choose a reason for hiding this comment

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

LGTM for GatherOp::verify()

lib/Dialect/IR/TcpOps.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@zezhang zezhang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this tricky case.

@matthewfl matthewfl merged commit 2f129aa into cruise-automation:main Sep 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants