-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fuse inputs with mlir #3010
Fuse inputs with mlir #3010
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3010 +/- ##
===========================================
- Coverage 92.21% 92.21% -0.01%
===========================================
Files 493 493
Lines 19725 19730 +5
===========================================
+ Hits 18190 18194 +4
- Misses 1535 1536 +1 ☔ View full report in Codecov by Sentry. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
@@ -43,5 +46,31 @@ void sort_params(std::vector<instruction_ref>& params) | |||
})); | |||
} | |||
|
|||
std::vector<instruction_ref> | |||
find_inputs(const std::unordered_map<instruction_ref, instruction_ref>& map_ins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function needs a descriptive comment or no one else will ever be able to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Brian about the need for comments and the need for filtering.
(I'm not going to block this because this isn't my neam, but)
Co-authored-by: Umang Yadav <[email protected]>
@@ -245,6 +245,21 @@ struct MIGRAPHX_EXPORT module | |||
const std::vector<instruction_ref>& splits1, | |||
const std::vector<instruction_ref>& splits2) const; | |||
|
|||
// Fuse the instruction into the module by inserting the instructions and | |||
// parameters for any missing inputs. | |||
std::vector<instruction_ref> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some unit-tests
std::transform(names.begin(), names.end(), std::back_inserter(result), [](const auto& p) { | ||
return p.second; | ||
}); | ||
assert(not sub or result.size() == sub->get_parameter_shapes().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If sub == nullptr
you can just do early return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
sub == nullptr
you can just do early return
Early return where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just when it starts the body of find_inputs()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then that will skip getting the parameters. Its meant to be optional. If sub
is null then it will assume all parameters come from the submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall but need to add unit & verify tests. I am not sure how to do verify tests with ENV flag though.
We can add the ENV var to MLIR jenkins job. We already do this to enable MLIR for everything. |
This will fuse the inputs but only when using the
MIGRAPHX_ENABLE_MLIR_INPUT_FUSION
env variable.