You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/11/13 13:21:28 UTC

[GitHub] [tvm] guberti opened a new issue, #13364: [Bug] [microTVM] Native schedules for microTVM have worse accuracy than expected

guberti opened a new issue, #13364:
URL: https://github.com/apache/tvm/issues/13364

   When the microTVM team was working on our submission for MLPerf Tiny, we noticed the `kws` model had slightly worse accuracy than expected (we classified `897/1000` samples correctly, while folks are expected to classify [at least `900/1000` correctly](https://github.com/mlcommons/tiny/tree/master/benchmark)). This issue does not occur when running with CMSIS-NN schedules.
   
   Despite a bit of looking, it is unclear to me what the issue is. It's possible that it's a padding issue, or maybe a problem with requantization? Total guesses.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [tvm] tmoreau89 commented on issue #13364: [Bug] Padded, quantized convolution operators give incorrect results on edges

Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on issue #13364:
URL: https://github.com/apache/tvm/issues/13364#issuecomment-1314063821

   CC @ibsidorenko as an FYI


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [tvm] guberti commented on issue #13364: [Bug] Padded, quantized convolution operators give incorrect results on edges

Posted by GitBox <gi...@apache.org>.
guberti commented on issue #13364:
URL: https://github.com/apache/tvm/issues/13364#issuecomment-1315500127

   > This is not a bug from QNN right? We already do pad by input zp https://github.com/apache/tvm/blob/main/src/relay/qnn/op/convolution.cc#L250
   
   Unfortunately, it seems I may have spoken too soon :sweat_smile:. I took a closer look, and @masahi is right. I got confused because microTVM does not pass a constant to `pad`, which I assumed meant the padded constant was zero (as [zero is the default](https://tvm.apache.org/docs/reference/api/python/topi.html#tvm.topi.nn.pad)). 
   
   https://github.com/apache/tvm/blob/034dc67d032aac3b848e15a87a7fbb5b72a0b909/python/tvm/topi/arm_cpu/mprofile/dsp/conv2d.py#L80-L82
   
   However, I didn't actually check the generated code to make sure what occurred. Looking more closely at the generated code shows that padding-like operations happen in two places. A padding-like operation does occur inside the generated `conv2d` function code:
   
   ```c
     for (int32_t i1 = 0; i1 < 50; ++i1) {
       for (int32_t i2 = 0; i2 < 50; ++i2) {
         for (int32_t i3 = 0; i3 < 8; ++i3) {
           int32_t cse_var_1 = (((i1 * 400) + (i2 * 8)) + i3);
           ((int8_t*)padded_data)[cse_var_1] = ((int8_t*)p0)[cse_var_1];
         }
       }
     }
   ```
   However, this does not do anything, and does not need to. Because of the QNN legalization steps, a function like this runs beforehand, and is what actually *does* the padding.
   
   ```c
   TVM_DLL int32_t tvmgen_default_fused_nn_pad_cast(void* args, int32_t* arg_type_ids, int32_t num_args, void* out_ret_value, int32_t* out_ret_tcode, void* resource_handle) {
     void* arg_p0 = (((TVMValue*)args)[0].v_handle);
     int32_t arg_p0_code = arg_type_ids[0];
     void* arg_T_cast = (((TVMValue*)args)[1].v_handle);
     int32_t arg_T_cast_code = arg_type_ids[1];
     void* p0 = (((DLTensor*)arg_p0)[0].data);
     void* arg_p0_shape = (((DLTensor*)arg_p0)[0].shape);
     void* arg_p0_strides = (((DLTensor*)arg_p0)[0].strides);
     int32_t dev_id = (((DLTensor*)arg_p0)[0].device.device_id);
     void* T_cast = (((DLTensor*)arg_T_cast)[0].data);
     void* arg_T_cast_shape = (((DLTensor*)arg_T_cast)[0].shape);
     void* arg_T_cast_strides = (((DLTensor*)arg_T_cast)[0].strides);
     if (!(arg_p0_strides == NULL)) {
     }
     if (!(arg_T_cast_strides == NULL)) {
     }
     for (int32_t ax0_ax1_fused_ax2_fused = 0; ax0_ax1_fused_ax2_fused < 9409; ++ax0_ax1_fused_ax2_fused) {
       for (int32_t ax3_inner = 0; ax3_inner < 3; ++ax3_inner) {
         int32_t cse_var_1 = (ax0_ax1_fused_ax2_fused % 97);
         ((int16_t*)T_cast)[((ax0_ax1_fused_ax2_fused * 3) + ax3_inner)] = ((int16_t)(((ax0_ax1_fused_ax2_fused < 9312) && (cse_var_1 < 96)) ? ((int8_t*)p0)[((((ax0_ax1_fused_ax2_fused / 97) * 288) + (cse_var_1 * 3)) + ax3_inner)] : (int8_t)-128));
       }
     }
     return 0;
   }
   ```
   
   At the end, you can see the `-128` zero point, as should be the case.
   
   There is still an accuracy issue with microTVM models, but my explanation for what's happening is wrong. I'm closing this issue, but will make a new one if/when I do track down the issue. Thanks to @masahi for making me check my work :).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [tvm] guberti commented on issue #13364: [Bug] [microTVM] Native schedules for microTVM have worse accuracy than expected

Posted by GitBox <gi...@apache.org>.
guberti commented on issue #13364:
URL: https://github.com/apache/tvm/issues/13364#issuecomment-1313903539

   # How does quantization work in general?
   
   Consider a **fused, quantized conv2d** operator, like those in the MLPerf Tiny (or any quantized MobileNetV1 model). In addition to having a kernel and bias, our operator has "three" extra parameters related to quantization (I'm simplifying slightly):
   
   - An **input zero point** (an `int32`)
   - One **requantization scale multiplier** per channel (a `float32`)
   - An **output zero point** (an `int32` that equals the input zero point for the next layer)
   - 
   
   $$3$$


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [tvm] guberti closed issue #13364: [Bug] Padded, quantized convolution operators give incorrect results on edges

Posted by GitBox <gi...@apache.org>.
guberti closed issue #13364: [Bug] Padded, quantized convolution operators give incorrect results on edges
URL: https://github.com/apache/tvm/issues/13364


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [tvm] masahi commented on issue #13364: [Bug] Padded, quantized convolution operators give incorrect results on edges

Posted by GitBox <gi...@apache.org>.
masahi commented on issue #13364:
URL: https://github.com/apache/tvm/issues/13364#issuecomment-1314512094

   This is not a bug from QNN right? We already do pad by input zp https://github.com/apache/tvm/blob/main/src/relay/qnn/op/convolution.cc#L250


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org