Skip to content

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Nov 1, 2023

Stack from ghstack (oldest at bottom):

In #112156 I added support for creating replacements on unbacked SymInts, so if you asserted that i0 == s0, we would replace i0 with s0 (only ever replacing unbacked with backed.)

However, if we have assertions involving only unbacked SymInts, we can also replace in this case! E.g., i0 == i1 or i0 == i1 * 12. The previous logic for generating replacements would reject these cases, because you're not allowed to replace unbacked with unbacked. Modifying the logic is not so easy though; ordinarily, we decide what substitution to prioritize by trying to replace the largest hinted symbol, but for unbacked integers we don't have this. To get around this problem, for now I only setup replacements for trivial symbol equals something else situations. Check the diff with whitespace ignored, the addition is quite small.

Signed-off-by: Edward Z. Yang ezyang@meta.com

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 1, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/112653

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (2 Unrelated Failures)

As of commit afd7f3e with merge base 68dead4 (image):

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ezyang added a commit that referenced this pull request Nov 1, 2023
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

ghstack-source-id: bfdb9bf
Pull Request resolved: #112653
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Nov 2, 2023
Signed-off-by: Edward Z. Yang <ezyangmeta.com>

ghstack-source-id: d313686
Pull Request resolved: #112653
@ezyang ezyang requested a review from Chillee November 2, 2023 13:26
@ezyang
Copy link
Contributor Author

ezyang commented Nov 2, 2023

cc @asmeurer

@ezyang ezyang requested a review from aakhundov November 2, 2023 13:26
@ezyang ezyang added ciflow/trunk Trigger trunk jobs on your pull request release notes: composability release notes category topic: bug fixes topic category labels Nov 2, 2023
@albanD albanD removed their request for review November 2, 2023 17:27
@asmeurer
Copy link
Collaborator

asmeurer commented Nov 7, 2023

I guess this is related to #112347 (comment)

I'm still learning how this code works, so it's hard to comment too much here. Is it really a good idea to replace in both directions? That means you don't have to be careful about a == b being different from b == a, but maybe it's also possible for this to result in replacements that go backwards from what you wanted?

(again, hard for me to know the answers to these questions because I'm still learning my way around all this)

@ezyang
Copy link
Contributor Author

ezyang commented Nov 7, 2023

Yeah, this simplification arose as desirable because we were trying to rewrite an integer so that it would pass through modulus checks.

Is it really a good idea to replace in both directions? That means you don't have to be careful about a == b being different from b == a, but maybe it's also possible for this to result in replacements that go backwards from what you wanted?

It's fine, for the equality replacements we are doing unification, so it doesn't matter which variable you end up with in the end, they're all in the same equivalence class.

(again, hard for me to know the answers to these questions because I'm still learning my way around all this)

Feel free to ask anything you need :)

Copy link
Contributor

@aakhundov aakhundov left a comment

Choose a reason for hiding this comment

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

This, together with the divisible_by workaround suggested in #112347, seems to help going through the reshape with -1 in the target shape in the internal model. Thanks!

@ezyang
Copy link
Contributor Author

ezyang commented Nov 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@facebook-github-bot facebook-github-bot deleted the gh/ezyang/2408/head branch November 10, 2023 15:24
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
…112653)

In pytorch#112156 I added support for creating replacements on unbacked SymInts, so if you asserted that `i0 == s0`, we would replace i0 with s0 (only ever replacing unbacked with backed.)

However, if we have assertions involving only unbacked SymInts, we can also replace in this case! E.g., `i0 == i1` or `i0 == i1 * 12`. The previous logic for generating replacements would reject these cases, because you're not allowed to replace unbacked with unbacked. Modifying the logic is not so easy though; ordinarily, we decide what substitution to prioritize by trying to replace the largest hinted symbol, but for unbacked integers we don't have this. To get around this problem, for now I only setup replacements for trivial symbol equals something else situations. Check the diff with whitespace ignored, the addition is quite small.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
Pull Request resolved: pytorch#112653
Approved by: https://github.com/aakhundov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: composability release notes category release notes: fx release notes category topic: bug fixes topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants