Skip to content

Fix get_free_symbol_uses for several nodes. #160134

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

Closed
wants to merge 1 commit into from

Conversation

laithsakka
Copy link
Contributor

@laithsakka laithsakka commented Aug 7, 2025

get_free_symbol_uses is used to know what unbacked symbols are used by a given node.
not having correct get_free_symbol_uses defined properly leads to :

  1. eliminating of some nodes due to not detection of any users. (See the added unit test)
  2. Incorrect topological sort.

Fix get_free_symbol_uses , NopKernel , ConcarKernel, InputsKerenl, external kernel.
for ComputedBuffer with NonOwningLayout its interesting case.
when layout is NonOwningLayout we need to access the actual view op base layout and use
detect symbols in it. Because when we codegen the ComputedBuffer we uses those symbols.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben

Copy link

pytorch-bot bot commented Aug 7, 2025

🔗 Helpful Links

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

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

❌ 9 New Failures

As of commit 55d60f4 with merge base 5ed4f91 (image):

NEW FAILURES - The following jobs have failed:

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79826917

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79826917

pytorch-bot bot pushed a commit that referenced this pull request Aug 8, 2025
Summary:

todo

Test Plan:
todo

Rollback Plan:

Differential Revision: D79826917
laithsakka added a commit to laithsakka/pytorch that referenced this pull request Aug 8, 2025
…OwningLayout , NopKernel , ConcarKernel, InputsKerenl (pytorch#160134)

Summary: Pull Request resolved: pytorch#160134

Test Plan:
todo

Rollback Plan:

Differential Revision: D79826917
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79826917

@laithsakka laithsakka changed the title Fix try 2 Fix get_free_symbol_uses for several nodes. Aug 9, 2025
laithsakka added a commit to laithsakka/pytorch that referenced this pull request Aug 9, 2025
Summary: Pull Request resolved: pytorch#160134

Test Plan:
todo

Rollback Plan:

Differential Revision: D79826917
laithsakka added a commit to laithsakka/pytorch that referenced this pull request Aug 9, 2025
Summary: Pull Request resolved: pytorch#160134

Test Plan:
todo

Rollback Plan:

Differential Revision: D79826917
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79826917

Summary: Pull Request resolved: pytorch#160134

Test Plan:
todo

Rollback Plan:

Differential Revision: D79826917
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79826917

@laithsakka laithsakka requested a review from pianpwk August 9, 2025 03:07
Copy link
Contributor

@bobrenjc93 bobrenjc93 left a comment

Choose a reason for hiding this comment

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

very nice

@laithsakka
Copy link
Contributor Author

@pytorchbot merge -i "not related faluire pre exisiting"

Copy link

pytorch-bot bot commented Aug 9, 2025

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: unrecognized arguments: not related faluire pre exisiting

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci,cherry-pick} ...

Try @pytorchbot --help for more info.

@laithsakka
Copy link
Contributor Author

@pytorchbot merge -i

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 9, 2025
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@laithsakka
Copy link
Contributor Author

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 1 checks: Lint / lintrunner-noclang / linux-job

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

@malfet
Copy link
Contributor

malfet commented Aug 10, 2025

@pytorchbot revert -m "No, those are not pre-existing, see https://hud.pytorch.org/hud/pytorch/pytorch/df55ec7d4b35f6d21691e9dd41c82f27de762948/1?per_page=50&name_filter=lint&mergeEphemeralLF=true" -c ignoredsignal

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@laithsakka your PR has been successfully reverted.

@pytorchmergebot pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels Aug 10, 2025
@malfet
Copy link
Contributor

malfet commented Aug 10, 2025

cc: @clee2000 looks like this PR was a huge TD miss: i.e. previously it only complained about linter, but once landed (and reverted with no-td) it started to fail a number of jobs.

@laithsakka
Copy link
Contributor Author

laithsakka commented Aug 11, 2025

replaced with #160314

@laithsakka
Copy link
Contributor Author

replaced

@laithsakka laithsakka closed this Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants