Skip to content

[Inductor] Restrict ND tiling analysis to MemoryDeps #144497

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.

Sign up for GitHub

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

Jump to bottom
Closed
blaine-rister wants to merge 2 commits into from
Closed

[Inductor] Restrict ND tiling analysis to MemoryDeps #144497

blaine-rister wants to merge 2 commits into from

Conversation

@blaine-rister
Copy link
Contributor

@blaine-rister blaine-rister commented Jan 9, 2025 •
edited
Loading

Issue

#137243 introduced a feature where the ND tiling algorithm analyzes memory dependencies. It iterates over all Dep's of the kernel. However, the analysis is only applicable to MemoryDep instances, which are a subclass of Dep. In particular, it doesn't work for StarDep's, for the reasons described here: https://github.com/pytorch/pytorch/blob/main/torch/_inductor/codegen/simd.py#L1653

Fix

This PR changes the algorithm to only iterate over MemoryDep instances.

Testing

Parameterized an existing test for torch.bucketize to also run with ND tiling. This test emits a node with StarDep's. Without this PR, the compiler would crash on this test case.

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

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 9, 2025 •
edited
Loading

🔗 Helpful Links

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

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 4ca7680 with merge base e6b9e67 (image):

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

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

@blaine-rister blaine-rister force-pushed the brister/nd_tiling_stardep branch from f04bcbb to 8d477e3 Compare January 9, 2025 21:33
blaine-rister
blaine-rister commented Jan 9, 2025
View reviewed changes
test/inductor/test_torchinductor.py
boundaries = torch.tensor([-0.9, -0.8, 0.1, 0.2, 0.5, 0.9])

for out_int32 in [True, False]:
for right in [True, False]:
Copy link
Contributor Author

@blaine-rister blaine-rister Jan 9, 2025

Choose a reason for hiding this comment

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

Unrelated to this PR: I'm not sure why this test iterates over various values of out_int32 and right, but then hard-codes them to True and False, respectively before actually compiling the model. Should we change this?

@blaine-rister blaine-rister changed the title [Inductor] Fix ND tiling with stardeps [Inductor] Restrict ND tiling analysis to MemoryDeps Jan 9, 2025
blaine-rister
blaine-rister commented Jan 9, 2025
View reviewed changes
@blaine-rister
Copy link
Contributor Author

blaine-rister commented Jan 10, 2025

Looks like some infra issues prevented a lot of tests from running. I'll rebase this and test again.

@blaine-rister
Copy link
Contributor Author

blaine-rister commented Jan 10, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jan 10, 2025

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jan 10, 2025

Successfully rebased brister/nd_tiling_stardep onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout brister/nd_tiling_stardep && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the brister/nd_tiling_stardep branch from e37fb4a to 4ca7680 Compare January 10, 2025 21:22
@blaine-rister
Copy link
Contributor Author

blaine-rister commented Jan 11, 2025

@pytorchbot merge -i

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

pytorchmergebot commented Jan 11, 2025

Merge started

Your change will be merged while ignoring the following 1 checks: pull / linux-jammy-py3-clang12-executorch / test (executorch, 1, 1, lf.linux.2xlarge, unstable)

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 brister/nd_tiling_stardep branch February 12, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eellison eellison eellison approved these changes

@jansel jansel Awaiting requested review from jansel

@arui-meta arui-meta Awaiting requested review from arui-meta

@shunting314 shunting314 Awaiting requested review from shunting314

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@blaine-rister @pytorchmergebot @eellison