-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay][Topi] Disable conv NHWC pack int8. #4038
Conversation
Ping @llyfacebook A gentle ping :) Please let me know your thoughts about this. |
|
Yes, the test failed when I had different input and output channels. You can also reproduce the issue by using the test case in this PR and uncommenting the dispatch to nhwc pack compute. |
@llyfacebook Did you get a chance to take a look? If you are busy, is it ok if we comment the code for now and open an issue to track? We can uncomment it back once we have good understanding of the issue and we can also add a test that exercises the commented code. What do you say? |
A gentle ping @llyfacebook :) |
Sorry for the delay, @anijain2305 . It seems the topi.testing.conv2d_nhwc_python assumes the W layout is HWIO. I think I need to change the weight packing to make it work for HWIO. Go ahead to comment it out and open an issue and assign it to me. Thanks. |
@anijain2305 Can you add a FIXME there? Then open an issue for @llyfacebook? Thanks. |
4254c70
to
8c4375d
Compare
8c4375d
to
d7fd677
Compare
@zhiics Both done. Please have a look now. |
* master: (51 commits) [QNN][TFLite] Parsing QNN Add op. Adding MobilenetV2. (apache#4142) [CI] Pin NNPack pthreadtools version (apache#4152) Fix typo (apache#4144) [Relay][Frontend][TF] Add tensor array ops (apache#3798) [relay][vm] Separate VM runtime with executable (apache#4100) [PATCH] Fix undefined __floatdihf in libtvmruntime.so on aarch64. (apache#4119) [DOCKER] Pin torchvision==0.4.1 (apache#4140) [TOPI][x86] Cascade lake support. (apache#4123) [Relay] Improve build error when no lowered funcs are produced (apache#4132) [RUNTIME] Refactor object python FFI to new protocol. (apache#4128) Update PULL_REQUEST_TEMPLATE.md Adding support for dequantizing from int32 to float32. (apache#4130) [Relay][Training] Add and fix gradients (apache#4126) [QNN] Change default rouning to UPWARD. (apache#4131) Fix infer type of kernel in dense. (apache#4125) [Relay][AlterOpLayout] NHWC to NCHWc pad operator. (apache#4103) [ARITH] Fix lowering of floormod(x, y) != 0 (apache#4127) [RFC][RUNTIME] Introduce new object protocol. (apache#4115) [Relay][Topi] Disable conv NHWC pack int8. (apache#4038) Update task_cpp_unittest.sh ...
Currently, _declaration_conv_nhwc_pack expects (NHWC, HWOI). Since, TOPI has no way of identifying the kernel layout, this code is triggered for TFLite which has (NHWC, HWIO).
I tried to investigate if it is a simple fix. However, I am somewhat confused. The test "topi/tests/python/test_topi_conv2d_nhwc_pack_int8.py" sets up the conv with HWIO layout instead of HWOI. The failure was not triggered because input_channels = output_channels in the test. When I change the test, it starts failing. Currently, there is no test from the Relay side that exercises the commented code in this PR.
So, for now, I disabled the dispatch to that nwhc_pack compute. @llyfacebook Will you be to take a look at this? You can regenerate the error by commenting out the changes in conv2d.py
Why I am looking into this? - This causes issue while executing quantized TFLite model on Skylake machine (-mcpu=skylake-avx512).
@yzhliu @zhiics @tqchen