inductor: fix for functional_collectives.wait() followed by view()

[bdhirsh]

Potential fix for #118759. See the issue linked for more diagnosis / explanation of this (tentative) fix. Feedback welcome!

Stack from ghstack (oldest at bottom):

• DTensor: avoiding crashing on dynamic shapes in a few places #123349
• Dynamo: support proxying tensor subclass constructors, including with non-fx types #123350
• make sure dynamo doesn't inline DTensor __new__ or __torch_dispatch__ #123347
• Fakeifying views shouldnt create symbols when dynamic=False #123348
• fix correctness for dynamo inlining RangeVariable __contains__ #122751
• compile: ban mutations on non-compositional uses of as_strided #122502
• -> inductor: fix for functional_collectives.wait() followed by view() #118802

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang @d4l3k @voznesenskym @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @rohan-varma @aakhundov

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❌ 2 New Failures, 3 Unrelated Failures

As of commit 25e9a62 with merge base a631461 ():

NEW FAILURES - The following jobs have failed:

• inductor / cuda12.1-py3.10-gcc9-sm86 / test (inductor_distributed, 1, 1, linux.g5.12xlarge.nvidia.gpu) (gh)
  test/distributed/_tensor/test_dtensor_compile.py::TestDTensorCompile::test_tp_compile_comm_reordering_use_native_funcol_False
• inductor / cuda12.1-py3.10-gcc9-sm86 / test (inductor, 1, 1, linux.g5.4xlarge.nvidia.gpu) (gh)
  test/distributed/_tensor/test_dtensor_compile.py::TestDTensorCompile::test_tp_compile_comm_reordering_use_native_funcol_False

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

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

• pull / linux-focal-py3.12-clang10 / test (default, 3, 3, linux.2xlarge) (gh)
  test_binary_ufuncs.py::TestBinaryUfuncsCPU::test_logaddexp_cpu_complex128
• pull / linux-jammy-py3-clang12-executorch / test (executorch, 1, 1, linux.2xlarge) (gh)
  Delegation fp32 error

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

• inductor / rocm6.0-py3.8-inductor / test (inductor, 1, 1, linux.rocm.gpu.2, unstable) (gh)
  test/distributed/_tensor/test_dtensor_compile.py::TestDTensorCompile::test_tp_compile_comm_reordering_use_native_funcol_False

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

[yf225]

It looks good in general, would be curious if there is any failed tests from CI

[wconstab]

I don't see a problem with the fix. But I don't have confidence either bc I don't understand the IR very well or the changes that were made the FC inductor implementation as it evolved... Maybe @yifuwang is a good person to review.

[yifuwang]

Was this test broken? Curious since I don't think your change affects this test.

[bdhirsh]

Hmm I don't think it was. wait() was just broken in some situations, like when there is a view right after it, but this PR technically changed the name of the buffer of all downstream users of wait_tensor (so use the output of wait_tensor instead of the input)

[yifuwang]

Just a heads up - we've switch to native funcol which doesn't use the Inductor IR you modified. The test you introduced is still legit though.

We are planning to remove the old funcol ops and IRs soon. In case you want the test to also cover the old funcol in the meanwhile, you can decorate it with @run_with_both_funcol_impls.

[bdhirsh]

@pytorchbot merge

[pytorchmergebot]

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

[bdhirsh]

@pytorchbot merge

[pytorchmergebot]

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

[pytorchmergebot]

Merge failed

Reason: 5 jobs have failed, first few of them are: inductor, trunk, linux-binary-manywheel, linux-binary-libtorch-cxx11-abi, linux-binary-libtorch-pre-cxx11

Details for Dev Infra team

Raised by workflow job

[github-actions]

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.