Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier#40232
Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier#40232kredd2506 wants to merge 2 commits into
Conversation
| } | ||
| )"; | ||
| TF_ASSERT_OK_AND_ASSIGN(std::unique_ptr<HloModule> module, | ||
| ParseAndReturnUnverifiedModule(kModuleStr)); |
There was a problem hiding this comment.
I think here you can still use ParseAndReturnVerifiedModule?
|
Good point, updated to use |
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 892874014
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 893321355
|
Just FYI, your new check exposed a test that had invalid HLO. I am fixing that and then your PR will get merged. |
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR #40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 893321355
… HLO verifier Imported from GitHub PR openxla/xla#40232 ## Summary Fixes #40191 `VerifyAsynchronousInstructionPairs` checks that async Start/Done instructions are properly connected for all async op types except `kAllGatherStart/kAllGatherDone`. This adds the missing verification, following the same pattern used for `kAllReduceStart/kAllReduceDone`. ## Changes - `hlo_verifier.cc`: Add `kAllGatherStart` and `kAllGatherDone` cases to the async pair verification switch - `hlo_verifier_test.cc`: Add tests for valid AllGather Start/Done pair and invalid multiple-Done case ## Test plan - [ ] CI verification - [x] Tests follow existing AllReduce verifier test patterns - [x] Follow Google C++ style Copybara import of the project: -- 30b3bd3e40e9fbd25f5336febefe6f06fa6e0346 by Manish Reddy <kreddy.manish@gmail.com>: Add missing kAllGatherStart/kAllGatherDone verification to HLO verifier. Fixes #40191 -- 8a46e9c7312dad8f8f47e52c9b1548456cee934f by Manish Reddy <kreddy.manish@gmail.com>: Use ParseAndReturnVerifiedModule for valid AllGather test case. Merging this change closes #40232 PiperOrigin-RevId: 893456297
We are migrating things away of mlir::AffineExpr mostly to use our own data structure that can modify as we want. As it does not depend on MLIRContext mutex directly, we also believe that it could be faster (some benchmarks for previous migration could be found in cl/882547562). Most interesting differences: - I had to modify third_party/tensorflow/compiler/xla/codegen/tiling/experimental/tiling_space.cc to add all the dimensions before creating the symbols because they require num_dims at creation. The multi-pass approach in TilingSpace::Create is required by the SymbolicExpr variable indexing convention. The potential performance impact was deemed acceptable for this migration by pifon@. - We did modify the output of a few tests, we are not canonicalizing automatically in expression creation, and this mostly differentiate with AffineExpr that v1 + v0 does not get canonicalized to v0 + v1. These changes are expected and do not affect the logic's correctness. We could directly call to SymbolicExpr::Canonicalize() if we want to enforce this, for now I'm leaving it as it is. FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#40232 from kredd2506:fix-verifier-allgather-async-check 8a46e9c7312dad8f8f47e52c9b1548456cee934f PiperOrigin-RevId: 888715565
|
Thanks for fixing that! When I first saw the failing test I was reviewing my own logic thinking I'd broken something — but your fix makes sense. The verifier was assuming shapes were arrays/tuples without actually checking, and the nested tuple structure for Nice catch extending the same fix to ReduceScatter too. Glad the async pair check surfaced these. Quick question — were these crashes exposed by an existing test that had invalid HLO, or was it previously valid HLO that just wasn't going through this verification path before? |
The crashes were exposed when I tried to adjust support_test.cc but got it wrong initially (creating invalid HLO that crashed instead of being rejected as invalid). After I fixed it for AllGather/AllGatherStart, I asked the KI to check whether there are more such bugs in this file, and it found ReduceScatter and fixed it for me. Only the test it created did not immediately have valid HLO, so I had to adapt it manually. |
Summary
Fixes #40191
VerifyAsynchronousInstructionPairschecks that async Start/Done instructionsare properly connected for all async op types except
kAllGatherStart/kAllGatherDone.This adds the missing verification, following the same pattern used for
kAllReduceStart/kAllReduceDone.Changes
hlo_verifier.cc: AddkAllGatherStartandkAllGatherDonecases to the async pair verification switchhlo_verifier_test.cc: Add tests for valid AllGather Start/Done pair and invalid multiple-Done caseTest plan