Skip to content

Conversation

@gchinora
Copy link
Contributor

@gchinora gchinora commented Oct 15, 2025

The whole purpose of the op-builders is to factor out the main functionalities behind the current onnx parsers and put them in a separate set of files (under the folder ./src/op/builder/ ) so they could be reused in different parts of the application. That is this is mainly a refactoring task; the passing of the already existing unit and verify tests should be the proof that the refactoring left the integrity of mgr intact.

related issue: migraphx-benchmark#203
covered parsers in this PR:

  • binary parsers (add, div, logical_and, logical_or, logical_xor, bitwise_and, mul, pow, prelu, sqdiff, sub)
  • concat (used by TF parser for ConcatV2)
  • tile (used by Onnx parser fort Tile)

…is essentially the same how add_instruction is called a few lines above
…hat the tests run not for only one kind of binary operator
…a default value for the 'options' parameter for an op-builder
@gchinora gchinora requested a review from causten as a code owner October 15, 2025 12:13
@gchinora gchinora marked this pull request as draft October 15, 2025 12:18
@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 92.13483% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/op/builder/op_builder.cpp 69.23% 4 Missing ⚠️
src/onnx/onnx_parser.cpp 83.33% 1 Missing ⚠️
...builder/include/migraphx/op/builder/op_builder.hpp 91.67% 1 Missing ⚠️
src/serialize.cpp 66.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4385      +/-   ##
===========================================
+ Coverage    92.18%   92.24%   +0.06%     
===========================================
  Files          560      563       +3     
  Lines        26641    27274     +633     
===========================================
+ Hits         24558    25158     +600     
- Misses        2083     2116      +33     
Files with missing lines Coverage Δ
src/onnx/include/migraphx/onnx/onnx_parser.hpp 100.00% <ø> (ø)
src/onnx/parse_binary_op.cpp 94.12% <100.00%> (+4.12%) ⬆️
src/onnx/parse_clip.cpp 100.00% <100.00%> (ø)
src/onnx/parse_gelu.cpp 100.00% <100.00%> (ø)
src/onnx/parse_generic_op.cpp 100.00% <100.00%> (ø)
src/onnx/parse_tile.cpp 100.00% <100.00%> (ø)
src/op/builder/clip.cpp 100.00% <ø> (ø)
src/op/builder/gelu.cpp 100.00% <100.00%> (ø)
.../op/builder/include/migraphx/op/builder/insert.hpp 100.00% <ø> (ø)
src/op/builder/pointwise.cpp 100.00% <100.00%> (ø)
... and 9 more

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…tor as it arrives; in case of the TF parser we have to modify the input vector that will be passed to the op-builder. We won't need the last item.
@gchinora gchinora requested review from kahmed10 and pfultz2 November 17, 2025 12:31
@causten causten requested a review from Copilot November 26, 2025 16:19
Copilot finished reviewing on behalf of causten November 26, 2025 16:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@gchinora gchinora requested review from kahmed10 and pfultz2 December 2, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants