Skip to content

[Inductor] Delay codegen for fallback arguments and improve typing#154371

Closed
benjaminglass1 wants to merge 12 commits intogh/benjaminglass1/85/basefrom
gh/benjaminglass1/85/head
Closed

[Inductor] Delay codegen for fallback arguments and improve typing#154371
benjaminglass1 wants to merge 12 commits intogh/benjaminglass1/85/basefrom
gh/benjaminglass1/85/head

Conversation

@benjaminglass1
Copy link
Collaborator

@benjaminglass1 benjaminglass1 commented May 26, 2025

Stack from ghstack (oldest at bottom):

Delays code generation for arguments to fallback ops. This is inspired by #155642, and likely fixes similar memory leaks.

Additionally, prepare for the next PR in the stack by tightening up typing on a cpp_wrapper interface that's only used in one (well-typed) place, as well as downstream effects of that change. In particular, this enabled:

  1. removing a number of now clearly unnecessary asserts
  2. adding a few more targeted asserts to validate the code's current assumptions
  3. removing some unneeded control flow in several functions

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

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented May 26, 2025

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 1d86eec with merge base 3819584 (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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

@benjaminglass1 benjaminglass1 requested a review from desertfire May 26, 2025 19:58
@benjaminglass1 benjaminglass1 marked this pull request as ready for review May 26, 2025 19:59
@benjaminglass1 benjaminglass1 self-assigned this May 26, 2025
@benjaminglass1 benjaminglass1 added the ciflow/trunk Trigger trunk jobs on your pull request label May 26, 2025
[ghstack-poisoned]
[ghstack-poisoned]
@benjaminglass1
Copy link
Collaborator Author

@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

@benjaminglass1
Copy link
Collaborator Author

@pytorchbot revert -c nosignal -m "Appears to have broken main"

@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 added a commit that referenced this pull request May 27, 2025
…I C-shim dispatching (#154371)"

This reverts commit 6169ca0.

Reverted #154371 on behalf of https://github.com/benjaminglass1 due to Appears to have broken main ([comment](#154371 (comment)))
@pytorchmergebot
Copy link
Collaborator

@benjaminglass1 your PR has been successfully reverted.

@pytorchmergebot pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels May 27, 2025
[ghstack-poisoned]
[ghstack-poisoned]
@benjaminglass1 benjaminglass1 changed the title [Inductor] Improve typing, and prepare for ABI-compatible AOTI C-shim dispatching [Inductor] Delay codegen for fallback arguments and improve typing Jun 13, 2025
@benjaminglass1
Copy link
Collaborator Author

@henrylhtsang When CI finishes running (I fully believe it will pass, but just to be sure), I'm ready for you to test this PR internally. I've addressed the one plausible memory leak I can see by delaying code generation on some arguments that could theoretically get leaked.

[ghstack-poisoned]
@henrylhtsang
Copy link
Contributor

@henrylhtsang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ghstack-poisoned]
@henrylhtsang
Copy link
Contributor

umm do I need to re-import

@benjaminglass1
Copy link
Collaborator Author

@henrylhtsang Yes, sorry, I caught a bug locally. Should be gtg now.

@henrylhtsang
Copy link
Contributor

@henrylhtsang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@henrylhtsang
Copy link
Contributor

Yeah LGTM cc @desertfire

@henrylhtsang
Copy link
Contributor

umm it seems to regress the latency by 1%. Any possible ideas?

@benjaminglass1
Copy link
Collaborator Author

@henrylhtsang Which latency? Compile-time latency?

@henrylhtsang
Copy link
Contributor

@henrylhtsang Which latency? Compile-time latency?

runtime latency

@benjaminglass1
Copy link
Collaborator Author

@henrylhtsang I've sent some questions to you offline; I'll put any conclusions we come to in this PR.

[ghstack-poisoned]
@henrylhtsang
Copy link
Contributor

LGTM, false alarm

@benjaminglass1
Copy link
Collaborator Author

@henrylhtsang Excellent! Once you reimport the (cosmetic) changes I made, we should be GTG!

@henrylhtsang
Copy link
Contributor

@henrylhtsang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@benjaminglass1
Copy link
Collaborator Author

@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

@github-actions github-actions bot deleted the gh/benjaminglass1/85/head branch July 19, 2025 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants