-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add int4 & uint4 types to MigraphX #3378
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3378 +/- ##
===========================================
- Coverage 92.26% 92.24% -0.02%
===========================================
Files 500 500
Lines 20057 20061 +4
===========================================
Hits 18506 18506
- Misses 1551 1555 +4 ☔ View full report in Codecov by Sentry. |
MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_SHAPE_GENERATE_ENUM_TYPES) tuple_type | ||
MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_SHAPE_GENERATE_ENUM_TYPES) tuple_type, | ||
uint4_type, | ||
int4_type |
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.
We should have a macro to list these types.
@@ -115,6 +115,9 @@ migraphx_shape_datatype_t to_shape_type(shape::type_t t) | |||
case shape::x: return migraphx_shape_##x; | |||
MIGRAPHX_SHAPE_VISIT_TYPES(MIGRAPHX_DETAIL_SHAPE_CASE_CONVERT) | |||
#undef MIGRAPHX_DETAIL_SHAPE_CASE_CONVERT | |||
case shape::uint4_type: | |||
case shape::int4_type: | |||
break; |
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 wrong, the enum values should be exposed in the API and not throw an error.
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.
OK.
@@ -381,6 +383,8 @@ struct MIGRAPHX_EXPORT shape | |||
{ | |||
switch(t) | |||
{ | |||
case uint4_type: | |||
case int4_type: |
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.
The tuple visitor should not be called for these types. It should throw an error.
src/shape.cpp
Outdated
} | ||
|
||
shape_impl(shape::type_t t, std::vector<std::size_t> l) | ||
: m_type(t), m_lens(std::move(l)), m_standard(true) | ||
{ | ||
assert(t != shape::tuple_type); | ||
assert(t != shape::tuple_type && t != shape::uint4_type && t != shape::int4_type); |
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 assert shouldn't be changed here. It should be valid to construct a shape of int4_type otherwise whats the point of adding this type.
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.
Thanks. This one is definitely required.
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.
What do you mean?
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.
I meant you are correct. This constructor code, as you suggest, is definitely required for correct functioning.
(I had just quickly hacked those places to make sure the CI would pass the compile stage -- as some switch statements etc. were failing.) Thanks.
src/shape.cpp
Outdated
@@ -744,7 +752,7 @@ std::vector<shape> flatten(const std::vector<shape>& shapes) | |||
auto subs = flatten(s.sub_shapes()); | |||
result.insert(result.end(), subs.begin(), subs.end()); | |||
} | |||
else | |||
else if(s.type() != shape::uint4_type && s.type() != shape::int4_type) |
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 shouldn't be changed, we should still be able to flatten an tuples of int4_type.
In addition, we need to add unit tests for this type.
We also need a way to automatically insert the unpack_int4 for arguments in the onnx parser so we avoid passing it to unssuported operators(which is everything except unpack_int4). I wont be around the next couple of weeks to help design or review this, so this will have to wait until september to tackle this further. |
Also, we need update the bytes calculation to be the correct amount. We also need to throw when calling index calculations. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
Add int4 & uint4 types to MigraphX