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

Semantics of torch.symbolic_int min_value #3938

Open
kuhar opened this issue Jan 3, 2025 · 3 comments
Open

Semantics of torch.symbolic_int min_value #3938

kuhar opened this issue Jan 3, 2025 · 3 comments
Assignees

Comments

@kuhar
Copy link
Member

kuhar commented Jan 3, 2025

Based on the lowering in IREE:

 %292 = torch.symbolic_int "s0" {min_val = 2, max_val = 4095} : !torch.int

it seems that min_val = 2 has a non-obvious interpretation of simply being positive https://github.com/iree-org/iree/blob/26b24f2da8a26b8c17d3d9bb9836090e405d7709/compiler/plugins/input/Torch/InputConversion/BindSymbolicShapes.cpp#L108-L112 .

Could we clarify this in the op documentation here

Additionally, the operation annotates `min_val` and `max_val` attributes
denoting the range constraints for the dynamic dimension. This may be
useful for modeling runtime shape guards, or compile-time optimizations
based on the shape bounds (min, opt, max) on results of ops / regions.
or change the assembly format to make min_val an enum that spells out how to interpret the constants?

@stellaraccident
Copy link
Collaborator

Thanks Jakub and sorry you wasted time on this. We should also dig up the torch docs which describe the rationale that drives interpretation of the min_val and include it for posterity. I have read it but don't have it directly at hand. But I'm sure you won't be the last one to get confused by this (since the definition is confusing).

@sjain-stanford
Copy link
Member

Thanks for raising this @kuhar, and thanks for looping me in @stellaraccident .

This was a bit of a transient state within TorchDynamo's design for dynamic shapes where, for a while, min_val would be set to 2 to explicitly avoid the 0/1 specialization issue (as @stellaraccident hinted at above) and ambiguities in broadcasting semantics.

This doc (written by PT devs) sheds more light on the issue, but doesn't necessarily prescribe any concrete solution. From what I recall, this was subsequently fixed in later PT releases with other robust mechanisms which allow min_val to simply resolve to 0 unless constrained by the user.

This seems like an intermediate state that you're in and should resolve to an all positive range (0, INT_MAX) in more recent PT releases. I'm happy to add some breadcrumbs in the ops's documentation for posterity. LMK if you have a alternate proposal.

* ``exported_program.range_constraints`` describes the ranges of each symbol
  appearing in the graph. In this case, we see that ``s0`` has the range
  [0, int_oo]. For technical reasons that are difficult to explain here, they are
  assumed to be not 0 or 1. This is not a bug, and does not necessarily mean
  that the exported program will not work for dimensions 0 or 1. See
  `The 0/1 Specialization Problem <https://docs.google.com/document/d/16VPOa3d-Liikf48teAOmxLc92rgvJdfosIy-yoT38Io/edit?fbclid=IwAR3HNwmmexcitV0pbZm_x1a4ykdXZ9th_eJWK-3hBtVgKnrkmemz6Pm5jRQ#heading=h.ez923tomjvyk>`_
  for an in-depth discussion of this topic.

(src)

@kuhar
Copy link
Member Author

kuhar commented Jan 3, 2025

I'm happy to add some breadcrumbs in the ops's documentation for posterity

+1

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

No branches or pull requests

3 participants