Skip to content
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

[Quantization] Add symmetric with power2 scale quantization schema #3437

Conversation

@mciprian13
Copy link
Contributor

commented Aug 20, 2019

Summary
Add new quantization schema (named "SymmetricWithPower2Scale").
The new quantization schema is a particular quantization schema which:

  • is symmetric (offset = 0)
  • the floating point scale if forced to be a power of 2 (scale = 2 ^ exp for example 0.5, 0.125, etc)

For the previous quantization schemes the main approximation was:

  • (x * float_scale + offset) -> ((x >> pre_scale * integer_scale) >> post_scale) + offset

For the new quantization schema the approximation has a particular, more simpler form:

  • (x * float_scale + offset) -> x * integer_scale for positive exponent
  • (x * float_scale + offset) -> x >> post_scale for negative exponent

The new quantization schema:

  • is expected to be less accurate (in terms of arithmetic approximation of floating-point operations)
  • is expected to be simpler (arithmetic complexity, number of operations)
  • can be viewed as another point in the spectrum of quantization schemes with different tradeoffs between accuracy and complexity (this is oriented more towards the "simple"/"less accurate" end)
  • has proved to be good enough in terms of accuracy for some networks (e.g. lenet_mnist, cifar10)
    where the network was sufficiently robust/redundant (1..2 % accuracy drop relative to floating-point)
  • the main point is that it allows the specialization of some the LIBJIT kernels with other open-source hand-optimized kernels/libraries for some specific architectures

This quantization schema requires the floating-point scales to preserve their bit-exact values during YAML round-trip. Therefore, I added a workaround for retrieving the floating-point scales bit-exactly during YAML round-trip.

Documentation
None.

Test Plan
Tests for new quantization schema.
Tests for verifying that floating-point numbers are preserved bit-exactly during YAML round-trip.

Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Aug 20, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@mciprian13 mciprian13 changed the title Add symmetric with power2 scale quantization schema [Quantization] Add symmetric with power2 scale quantization schema Aug 20, 2019

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Thanks @mciprian13 for contribution! This schema could be beneficial for multiple vendor.

Can you sign the CLA and make sure tests are passing.

@rdzhabarov rdzhabarov self-assigned this Aug 20, 2019

@mciprian13

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

It seems the tests are failing for other reasons than my code updates (some connection issues). Can you please verify (or rerun the tests if applicable)?
Thanks!

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Seems like everything is passing now. I'll get this reviewed today.

@beicy

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Please update the comments about "int rtn = (post > 0) ? (1 << (post - 1)) : 0;" . After that, I think it is good to merge :) Thanks!

@rdzhabarov
Copy link
Contributor

left a comment

Few comments. Looks great!

lib/Quantization/Serialization.cpp Outdated Show resolved Hide resolved
lib/Quantization/Serialization.cpp Outdated Show resolved Hide resolved
lib/Quantization/Serialization.cpp Show resolved Hide resolved
tools/loader/Loader.cpp Show resolved Hide resolved
@@ -338,6 +360,11 @@ TensorQuantizationParams chooseQuantizationParams(float min, float max,
nudgedZeroPoint = static_cast<int32_t>(round(initialZeroPoint));
}

// For SymmetricWithPower2Scale, round scale to nearest higher power of 2.
if (schema == quantization::Schema::SymmetricWithPower2Scale) {
scale = std::exp2(std::ceil(std::log2(scale)));

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Aug 23, 2019

Contributor

do we have a test which verifies that for SymmetricWithPower2Scale scale is indeed power of 2?

This comment has been minimized.

Copy link
@mciprian13

mciprian13 Aug 28, 2019

Author Contributor

There is an assert in the same function of code which does that all the time.
I did however add an unit test to make the verification: chooseQuantizationSymmetricWithPower2Scale

This comment has been minimized.

Copy link
@mciprian13

mciprian13 Aug 29, 2019

Author Contributor

While adding the last unit test I found a bug (or corner case) in the function "chooseQuantizationParams":

  • for the previous "Symmetric" schema (and also for the newly added "SymmetricWithPower2Scale"), when qTy=UInt8QTy (qmin = 0) we get division by zero in the following line of code:
    double rmin = min / (double)qmin;

What should be do about this? This problem was already there and was not exposed by unit tests. Do you think we should exclude this case by putting an assert in the function?

Thanks!

This comment has been minimized.

Copy link
@rdzhabarov

rdzhabarov Aug 29, 2019

Contributor

Interesting. Thanks for flagging this!

Do you think we should exclude this case by putting an assert in the function?

I think that would be reasonable thing to add.

Symmetric with Uint basically means that it's only non negative (non positive, depending on a scale sign) numbers that could be represented, which for majority, if not all, networks would be a weird range.

@rdzhabarov
Copy link
Contributor

left a comment

LGTM. Can you look into ASAN failures.

@facebook-github-bot
Copy link

left a comment

@rdzhabarov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Remove test which causes divison by zero. Assert corner case of symme…
…tric quantization with unsigned range.
@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Internal tests are failing with

Quantization.chooseQuantizationSymmetricWithPower2Scale
glow/glow/lib/Quantization/Base/Base.cpp:305:23: runtime error: division by zero
    #0 0x7fa30255fc9c in glow::quantization::chooseQuantizationParams(float, float, glow::quantization::Schema, glow::ElemKind) glow/glow/lib/Quantization/Base/Base.cpp:305
    #1 0x36f301 in glow::chooseQuantParamsPower2Scale(float, float, glow::ElemKind) glow/glow/tests/unittests/QuantizationTest.cpp:1370
    #2 0x36ef61 in glow::Quantization_chooseQuantizationSymmetricWithPower2Scale_Test::TestBody() glow/glow/tests/unittests/QuantizationTest.cpp:1378
    #3 0x7fa300b213de in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2473:29
    #4 0x7fa300b1244a in testing::Test::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2490:50
    #5 0x7fa300b1244a in testing::Test::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2480:6
    #6 0x7fa300b125a0 in testing::TestInfo::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2677:14
    #7 0x7fa300b125a0 in testing::TestInfo::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2651:6
    #8 0x7fa300b126b3 in testing::TestCase::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2802:31
    #9 0x7fa300b126b3 in testing::TestCase::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2787:6
    #10 0x7fa300b12dd3 in testing::internal::UnitTestImpl::RunAllTests() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:4739:46
    #11 0x7fa300b13084 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:2473:29
    #12 0x7fa300b13084 in testing::UnitTest::Run() /home/engshare/third-party2/googletest/master/src/googletest/googletest/src/gtest.cc:4333:55
    #13 0x7fa305533430 in RUN_ALL_TESTS() third-party-buck/platform007/build/googletest/include/gtest/gtest.h:2255
    #14 0x7fa305533307 in main common/gtest/LightMain.cpp:19

SUMMARY: UndefinedBehaviorSanitizer: float-divide-by-zero glow/glow/lib/Quantization/Base/Base.cpp:305:23 in 
"/usr/bin/time:
MaxMemory 99316
Inputs 0
Outputs 0"

Could you check this?

@mciprian13

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Are they still failing? It it was the previous ASAN problem I fixed it.

@facebook-github-bot
Copy link

left a comment

@rdzhabarov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Are they still failing? It it was the previous ASAN problem I fixed it.

I'll get it tested and get back.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@mciprian13 required tests are passing, could you resolve conflict on include/glow/Quantization/Base/Base.h and i'll get this merged.

@rdzhabarov
Copy link
Contributor

left a comment

Will merge after conflict resolution.

@facebook-github-bot
Copy link

left a comment

@rdzhabarov is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Sep 5, 2019

@rdzhabarov merged this pull request in 4554c5d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.