Add the heuristic of AddN op using reduce_sum op for parsing pb file (TF) #4251
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
kentqian
added
enhancement
…rflow 2. Update the concat op's logic to handle mixed static and dynamic shapes more effectively. 3. Replaced the dynamic dimension handling in concat with compute_common_dyn_dims.
…IGraphX into kqian/addn_with_reduce
generated addn tf file regarding to many elements
…ian/addn_with_reduce
kentqian
changed the title
Collaborator
This build is not recommended to merge 🔴 |
Collaborator
❌bert-mrpc-tf: ERROR - check error outputerror: unknown warning option '-Wnrvo' [-Werror,-Wunknown-warning-option]error: unknown warning option '-Wnrvo' [-Werror,-Wunknown-warning-option] 2025-08-29 00:03:16.115752: I tensorflow/core/platform/cpu_feature_guard.cc:210] This TensorFlow binary is optimized to use available CPU instructions in performance-critical operations. To enable the following instructions: SSE3 SSE4.1 SSE4.2 AVX AVX2 FMA, in other operations, rebuild TensorFlow with the appropriate compiler flags. Traceback (most recent call last): File "/src/AMDMIGraphX/tools/accuracy/accuracy_checker.py", line 359, in main() File "/src/AMDMIGraphX/tools/accuracy/accuracy_checker.py", line 306, in main graph = load_tf_graph(model_name) File "/src/AMDMIGraphX/tools/accuracy/accuracy_checker.py", line 300, in load_tf_graph graph_def.ParseFromString(f.read()) File "/usr/local/lib/python3.10/dist-packages/tensorflow/python/lib/io/file_io.py", line 116, in read self._preread_check() File "/usr/local/lib/python3.10/dist-packages/tensorflow/python/lib/io/file_io.py", line 77, in _preread_check self._read_buf = _pywrap_file_io.BufferedInputStream( tensorflow.python.framework.errors_impl.UnimplementedError: File system scheme '[local]' not implemented (file: '/new-saved-models/tf-misc/bert_mrpc1.pb') 🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴mask-rcnn: FAILED: MIGraphX is not within tolerance - check verbose output |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a heuristic optimization for parsing TensorFlow's AddN operation by switching from chain addition to reduce_sum for cases with many input tensors (>=5). Additionally, it enhances the concat operation to handle mixed static and dynamic shapes by converting everything to dynamic and then contracting back to static when possible.
- Implements a heuristic in AddN parsing that uses concat+reduce_sum+squeeze for 5+ inputs instead of chained addition
- Updates concat operation to handle mixed static/dynamic shapes by unifying to dynamic then contracting to static when possible
- Adds comprehensive test coverage for the new AddN heuristic with 10 input tensors
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tf/parse_addn.cpp | Implements heuristic to use reduce_sum approach for AddN with 5+ inputs |
| src/include/migraphx/op/concat.hpp | Enhances concat to handle mixed static/dynamic shapes and compute common dimensions |
| test/tf/tests/addn_test.cpp | Adds test case for AddN with 10 elements using the new heuristic |
| test/tf/gen_tf_pb.py | Adds generator function for the new AddN test case |
| test/tf/models/addn_with_many_elements_test.pb | Binary protobuf model for testing AddN with many elements |
| test/ref/add.cpp | Adds reference test for the reduce_sum approach to AddN |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if(args.size() == 1) | ||
| return args[0]; | ||
|
|
||
| if(args.size() < 5) // using heuristic when args exceed over 5 elements |
There was a problem hiding this comment.
The comment is inconsistent with the condition. The condition checks for 'less than 5' but the comment says 'when args exceed over 5 elements'. The comment should read 'using chain addition when args are less than 5 elements' or similar.
Suggested change
| if(args.size() < 5) // using heuristic when args exceed over 5 elements | |
| if(args.size() < 5) // using chain addition when args are less than 5 elements |
Comment on lines
+121
to
+129
| if(has_static and has_dynamic) | ||
| { | ||
| // Dynamic input shapes | ||
| for(std::size_t index = 0; index < inputs[0].ndim(); index++) | ||
| for(auto& input : inputs) | ||
| { | ||
| if(index != axis) | ||
| { | ||
| if(not std::all_of(inputs.begin(), inputs.end(), [&](const shape& s) { | ||
| return s.dyn_dims()[index] == inputs[0].dyn_dims()[index]; | ||
| })) | ||
| MIGRAPHX_THROW("CONCAT: all input dimensions should match in axis " + | ||
| std::to_string(index)); | ||
| } | ||
| } | ||
| std::size_t new_min = 0; | ||
| std::size_t new_max = 0; | ||
| for(const auto& input : inputs) | ||
| { | ||
| auto ddim = input.dyn_dims()[axis]; | ||
| new_min += ddim.min; | ||
| new_max += ddim.max; | ||
| if(input.dynamic()) | ||
| continue; | ||
| input = input.to_dynamic(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Modifying the inputs parameter directly can lead to unexpected side effects since it's passed by value but contains references to shapes. This modification affects the original shapes which may be used elsewhere. Consider creating a local copy of inputs or using a different approach to handle mixed shapes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Given a heuristic parsing solution for AddN op when trying to parse tf and support the concat op when having the mix shapes
Technical Details
Change the chain addition to reduce_sum op for parsing AddN op
If there is a mix of static and dynamic shapes, set everything to dynamic, then at the end, contract the shape back to static if possible. It also calculates the common non axis dims to bound the output. (Concat)
Test Plan
Add test cases in ref and tf/parse
Test Result
Submission Checklist