-
Notifications
You must be signed in to change notification settings - Fork 1k
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
src: attr: quantization refactor (part 1) #2270
base: dzarukin/refactor_quant_prereq
Are you sure you want to change the base?
Conversation
make test |
ad9b2d3
to
9b4efc6
Compare
make test |
9b4efc6
to
58b979e
Compare
make test |
58b979e
to
b196033
Compare
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, thanks for the refactor.
Just one thing about the mask: moving from a default of 0 to -1 seems slippery.
Now the dev will have to always check if the value is non default before doing a comparison to 0, or doing some binary operation (e.g. or/xor).
I think it might be worth to normalize the mask upon get_mask so that default value becomes an implementation detail.
b196033
to
a3f8667
Compare
3a6d823
to
004ca8b
Compare
make test |
004ca8b
to
4bdcad2
Compare
4bdcad2
to
0f09c05
Compare
make test |
0f09c05
to
e61f596
Compare
make test |
e61f596
to
1c44dbb
Compare
Every year around Christmas time something happens to quantization:
The whole point of this refactor is to move quantization attributes to C++ way of doing things - provide clear and simple interfaces to operate with objects and close members.
This part 1 covers scales which were not that bad in terms of interfaces but could be better with argument members which this part covers.
Interface for both scales and its new underlying object, quant_entry_t, provide getters for a mask, data_type and groups (no need to worry about ndims any longer!), as well as default values checks.
Initialization is still done through
set
,reset
was replaced withset
,Among legacy use-cases here are the main changes across sources:
common:
cpu:
mask != 0
which was the default value for common and non-initialized scales tomask > 0
as now the default mask is negative.gpu:
gemm_with_post_ops
implementations.tests:
Part 2 will cover zero-points.
Disclaimer: the change is somewhat fundamental, the bug leaks are highly possible even if all tests are passing. Feel free to report any if I missed something.