Fix assertion failure in gemm template lowering #146353
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/146353
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 5 Pending, 1 Unrelated FailureAs of commit b212a54 with merge base bc01918 ( NEW FAILURE - The following job has failed:
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. |
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
|
@pytorchbot label "topic: not user facing" |
dmpots
force-pushed
the
export-D68814625
branch
from
aaf91e7 to
2b2c301
Compare
dmpots
added a commit
to dmpots/pytorch
that referenced
this pull request
Feb 3, 2025
Summary: This commit fixes a crash in the gemm template lowering caused by hitting an [assert](https://github.com/pytorch/pytorch/blob/fd515e4f59bfa0ac9faa5185b7a02f3222c4cd08/torch/_inductor/codegen/common.py#L1181) that a buffer was previously removed. The assert triggers because in the first gemm lowering we use a local accumulation buffer, which causes the original buffer name to be added to the `removed_buffers` set. Then in the next gemm lowering we use the global buffer for accumulation, but that buffer name is already in the `removed_buffers` set. The fix is to add a unique suffix to the buffer name to avoid triggering the assert from different gemm lowerings. Test Plan: # Reduced test case ``` TRITON_LOCAL_BUILD=1 buck2 run 'fbcode//mode/opt' fbcode//caffe2/test/inductor:cpu_select_algorithm_cpu -- caffe2.test.inductor.test_cpu_select_algorithm.TestSelectAlgorithmCPU.test_local_and_global_accumulator_cpu_float32 ``` # Original failure ``` $ manifold get aitemplate/tree/aotinductor_cpu/915857944_1.input.predictor.disagg.remote_other /tmp/915857944_1.input.predictor.disagg.remote_other $ buck2 run @//mode/opt //deeplearning/aot_inductor/cpu:cli -- --local-model-path /tmp/915857944_1.input.predictor.disagg.remote_other --submodule remote_other --preset ads_second_stage_ranking_type_1 ``` Differential Revision: D68814625
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
|
@leslie-fang-intel and @frost-intel - could you please help take a look? we found another issue when trying autuotune from Meta internal models, and David comes up with this fix |
dmpots
force-pushed
the
export-D68814625
branch
from
2b2c301 to
7fed581
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
dmpots
added a commit
to dmpots/pytorch
that referenced
this pull request
Feb 6, 2025
Summary: This commit fixes a crash in the gemm template lowering caused by hitting an [assert](https://github.com/pytorch/pytorch/blob/fd515e4f59bfa0ac9faa5185b7a02f3222c4cd08/torch/_inductor/codegen/common.py#L1181) that a buffer was previously removed. The assert triggers because in the first gemm lowering we use a local accumulation buffer, which causes the original buffer name to be added to the `removed_buffers` set. Then in the next gemm lowering we use the global buffer for accumulation, but that buffer name is already in the `removed_buffers` set. The fix is to add a unique suffix to the buffer name to avoid triggering the assert from different gemm lowerings. Test Plan: # Reduced test case ``` TRITON_LOCAL_BUILD=1 buck2 run 'fbcode//mode/opt' fbcode//caffe2/test/inductor:cpu_select_algorithm_cpu -- caffe2.test.inductor.test_cpu_select_algorithm.TestSelectAlgorithmCPU.test_local_and_global_accumulator_cpu_float32 ``` # Original failure ``` $ manifold get aitemplate/tree/aotinductor_cpu/915857944_1.input.predictor.disagg.remote_other /tmp/915857944_1.input.predictor.disagg.remote_other $ buck2 run @//mode/opt //deeplearning/aot_inductor/cpu:cli -- --local-model-path /tmp/915857944_1.input.predictor.disagg.remote_other --submodule remote_other --preset ads_second_stage_ranking_type_1 ``` Differential Revision: D68814625
dmpots
force-pushed
the
export-D68814625
branch
from
7fed581 to
b48f0db
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
frost-intel
reviewed
Feb 6, 2025
dmpots
force-pushed
the
export-D68814625
branch
from
b48f0db to
fb64ee6
Compare
dmpots
added a commit
to dmpots/pytorch
that referenced
this pull request
Feb 7, 2025
Summary: This commit fixes a crash in the gemm template lowering caused by hitting an [assert](https://github.com/pytorch/pytorch/blob/fd515e4f59bfa0ac9faa5185b7a02f3222c4cd08/torch/_inductor/codegen/common.py#L1181) that a buffer was previously removed. The assert triggers because in the first gemm lowering we use a local accumulation buffer, which causes the original buffer name to be added to the `removed_buffers` set. Then in the next gemm lowering we use the global buffer for accumulation, but that buffer name is already in the `removed_buffers` set. The fix is to add a unique suffix to the buffer name to avoid triggering the assert from different gemm lowerings. Test Plan: # Reduced test case ``` TRITON_LOCAL_BUILD=1 buck2 run 'fbcode//mode/opt' fbcode//caffe2/test/inductor:cpu_select_algorithm_cpu -- caffe2.test.inductor.test_cpu_select_algorithm.TestSelectAlgorithmCPU.test_local_and_global_accumulator_cpu_float32 ``` # Original failure ``` $ manifold get aitemplate/tree/aotinductor_cpu/915857944_1.input.predictor.disagg.remote_other /tmp/915857944_1.input.predictor.disagg.remote_other $ buck2 run @//mode/opt //deeplearning/aot_inductor/cpu:cli -- --local-model-path /tmp/915857944_1.input.predictor.disagg.remote_other --submodule remote_other --preset ads_second_stage_ranking_type_1 ``` Differential Revision: D68814625
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
leslie-fang-intel
approved these changes
Feb 7, 2025
frost-intel
approved these changes
Feb 7, 2025
dmpots
force-pushed
the
export-D68814625
branch
from
fb64ee6 to
a8a5d97
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
Summary: This commit fixes a crash in the gemm template lowering caused by hitting an [assert](https://github.com/pytorch/pytorch/blob/fd515e4f59bfa0ac9faa5185b7a02f3222c4cd08/torch/_inductor/codegen/common.py#L1181) that a buffer was previously removed. The assert triggers because in the first gemm lowering we use a local accumulation buffer, which causes the original buffer name to be added to the `removed_buffers` set. Then in the next gemm lowering we use the global buffer for accumulation, but that buffer name is already in the `removed_buffers` set. The fix is to add a unique suffix to the buffer name to avoid triggering the assert from different gemm lowerings. Test Plan: # Reduced test case ``` TRITON_LOCAL_BUILD=1 buck2 run 'fbcode//mode/opt' fbcode//caffe2/test/inductor:cpu_select_algorithm_cpu -- caffe2.test.inductor.test_cpu_select_algorithm.TestSelectAlgorithmCPU.test_local_and_global_accumulator_cpu_float32 ``` # Original failure ``` $ manifold get aitemplate/tree/aotinductor_cpu/915857944_1.input.predictor.disagg.remote_other /tmp/915857944_1.input.predictor.disagg.remote_other $ buck2 run @//mode/opt //deeplearning/aot_inductor/cpu:cli -- --local-model-path /tmp/915857944_1.input.predictor.disagg.remote_other --submodule remote_other --preset ads_second_stage_ranking_type_1 ``` Differential Revision: D68814625
dmpots
force-pushed
the
export-D68814625
branch
from
a8a5d97 to
b212a54
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68814625 |
hl475
approved these changes
Feb 7, 2025
|
@pytorchbot merge |
Merge startedYour 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 |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cpu-py3 / test (default, 1, 3, windows.4xlarge.nonephemeral) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "the failed test is irrelevant with this PR" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This commit fixes a crash in the gemm template lowering caused by hitting an assert that a buffer was previously removed.
The assert triggers because in the first gemm lowering we use a local accumulation buffer, which causes the original buffer name to be added to the
removed_buffersset. Then in the next gemm lowering we use the global buffer for accumulation, but that buffer name is already in theremoved_buffersset.The fix is to add a unique suffix to the buffer name to avoid triggering the assert from different gemm lowerings.
Differential Revision: D68814625
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @amjames @desertfire @chauhang @aakhundov