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

[torch-mlir] Support lowering of aten constraint ops #3943

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

praveen-g-ctt
Copy link
Contributor

  1. aten::sym_constrain_range
  2. aten::sym_constrain_range_for_size
  3. aten::_assert_scalar

Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'd like to request a few small changes, and I'd also like to at least have a discussion for context on these ops and their relationship with torch.symbolic_int and torch.bind_symbolic_shape.

lib/Conversion/TorchToLinalg/Uncategorized.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Uncategorized.cpp Outdated Show resolved Hide resolved
lib/Conversion/TorchToLinalg/Uncategorized.cpp Outdated Show resolved Hide resolved
lib/Dialect/Torch/Transforms/DecomposeComplexOps.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@zjgarvey zjgarvey left a comment

Choose a reason for hiding this comment

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

Thanks for the work, Praveen.

After looking at the example IR, I think it is totally fine to merge this as-is. In future patches, it would be good to add some folders (e.g., if min and max are both %none's, just erase it). And if we see other examples where these ops could inform symbolic shape constraints in a useful way for codegen, we can make changes then.

test/Dialect/Torch/decompose-complex-ops.mlir Outdated Show resolved Hide resolved
@praveen-g-ctt
Copy link
Contributor Author

Thanks for the work, Praveen.

After looking at the example IR, I think it is totally fine to merge this as-is. In future patches, it would be good to add some folders (e.g., if min and max are both %none's, just erase it). And if we see other examples where these ops could inform symbolic shape constraints in a useful way for codegen, we can make changes then.

Sure Zach. Will add folders in future patches.

Copy link
Collaborator

@vivekkhandelwal1 vivekkhandelwal1 left a comment

Choose a reason for hiding this comment

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

LGTM

@praveen-g-ctt praveen-g-ctt merged commit fd65a66 into main Feb 5, 2025
3 checks passed
@praveen-g-ctt praveen-g-ctt deleted the torch_constrain_ops branch February 5, 2025 06:26
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.

5 participants