-
Notifications
You must be signed in to change notification settings - Fork 89
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 --fp8
option to quantize models in FP8 inside migraphx-driver
#2535
Conversation
We should also expose |
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.
LGTM
@@ -41,11 +41,19 @@ struct program; | |||
MIGRAPHX_EXPORT void quantize_fp16(program& prog, | |||
const std::vector<std::string>& ins_names = {"all"}); | |||
|
|||
MIGRAPHX_EXPORT void quantize_8bits(program& prog, |
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 dont think this needs to be declared in the header, its only used internally for quantize_int8
and quantize_fp8
.
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.
Done.
std::make_shared<std::vector<std::pair<float, float>>>(); | ||
std::shared_ptr<std::vector<float>> max_abs_vals = std::make_shared<std::vector<float>>(); | ||
|
||
auto calc_quant_params = [int8_quant_params, max_abs_vals, &t](std::size_t ins_index, | ||
std::vector<argument> args) { | ||
float quantized_range = (precision == shape::type_t::int8_type) ? 127.0 : 240.0; |
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.
Can you put this in another function? Ideally we should use a visit+numeric_limits to get the quantized range, but we can leave it as is for now.
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.
NOt making change for now.
src/quantization.cpp
Outdated
auto calc_quant_params = [int8_quant_params, max_abs_vals, &t](std::size_t ins_index, | ||
std::vector<argument> args) { | ||
float quantized_range = (precision == shape::type_t::int8_type) ? 127.0 : 240.0; | ||
auto calc_quant_params = [quant_8bit_params, max_abs_vals, quantized_range, &t]( |
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 can use [&]
capture for the lambda.
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.
Done.
src/quantize_8bits.cpp
Outdated
{ | ||
auto zero_point = m.add_literal(static_cast<int8_t>(param.second)); | ||
auto zero_point = m.add_literal( | ||
migraphx::literal{migraphx::shape{precision}, {static_cast<int8_t>(param.second)}}); |
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 cast is not needed anymore.
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.
Removed.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #2535 +/- ##
===========================================
- Coverage 91.50% 91.41% -0.09%
===========================================
Files 453 452 -1
Lines 17183 17153 -30
===========================================
- Hits 15723 15681 -42
- Misses 1460 1472 +12 ☔ View full report in Codecov by Sentry. |
{ | ||
continue; | ||
} | ||
else if(not starts_with(ins->name(), "@")) |
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 else
is redundant here.
Depends on #2506
Follows same scheme as Int8 quantization except it uses different scales compared to Int8.