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

[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