[CPU] Add fused activation to Conv2D kernel implementation.

[mciprian13]

Summary

• Added fused activation to CPU Conv2D kernel implementation.
• Added more fused activation types by adding an extra node/instruction member FusedActivationArgs for the activation parameters (e.g. min/max for Clip, alpha for LeakyRelu)
• Added addFusedActivation() utility in InstrBuilder.
• Propagated the fused activation where missing in some Conv2D transformations.
• Enabled a TFLite unit test which now works with the newest AvgPool implementation.

Test Plan

• Added unit tests in OperatorTest and GraphOptz.

[mciprian13]

@jfix71 Can you take a look on this? Thanks!

[leejaymin]

I have a quick question.

[leejaymin]

@mciprian13 Hello, I am Jemin. I wrote this code to prevent the fusion in case of asymmetric due to the offset. In asymmetric case, the offset of Conv. should be handled properly rather than replacing it by Relu's one. I wonder that you are aware of this issue.

[mciprian13]

@leejaymin I don't understand what is the problem of fusing an asymmetric activation? The libjit correctly handles an activation with asymmetric quantization. You can look in libjit_defs.h at the function libjit_activation_i32().

[leejaymin]

@mciprian13 Sorry for the ambiguous explanation. What I concern about is quantization parameters of convolution are substituted by ones of activation. In the symmetric case, A(partialSum) * matMul(conv) / outScale (conv) * InScale(relu) / outSacle (relu) is equal to A * matmul(conv) / outScale(relu). It is beacuse outScale (conv) is equivalent to inScale(relu). Therefore, the replacement of quantization parameters is fine. However, In the asymmetric case, there are offsets in Conv and activation. Instead of replacing them with the following code N->getResult().setType(activationNV.getType()), the original offset of activation should be properly computed in libjit_activation_i32(). Let me know If I was wrong.

[mciprian13]

@leejaymin Changing the quantization parameters (scale and offset) of the convolution is not a problem because the mathematics remains consistent. Doing so actually has benefits because the dynamic range of activation output is commonly smaller than the dynamic range of the activation input (conv output). Computing the convolution using the quantization parameters of the activation:

• ensures better representation (smaller quantization step and hence error) for those values which are not saturated by Relu/Clip
• saturates those values which will be later saturated by Relu/Clip (don't care)
• this is the approach used by TensorFlowLite which ALWAYS uses fused activations in their pre-quantized models
  From my experience fusing activations for quantized models always has benefits in terms of accuracy because it avoids an extra requantization (by using different quantization parameters at the interface between conv and activation).

[leejaymin]

@mciprian13 Thank you for the really thorough answer and for sharing me the insightful document. As you explained in the attached document, I agreed with the benefit of performance due to mathematical simpleness. One thing I still concern about is that S_y and O_y are forced to match S_a and O_a. Such force conversion could affect the final accuracy.

To figure out it, I tested accuracy variation by fusion on Glow OpenCL backend and I got the following results:

# File path: Glow/utils/imagenet_topk_accuracy_driver.py
# Use ONNX Resnet50 (Glow/utils/download_datasets_and_models.py) imagenet12 validation set (50,000)
# baseline accuracy (FP32) top1 76.08%

# Resnet50, Asymmetric, Fusion (Conv and Relu)
Completed running; Final Top-1/5 accuracy across 50000 images:
       Top-1 accuracy: 0.11

# Resnet50, Asymmetric nonFusion
Completed running; Final Top-1/5 accuracy across 50000 images:
       Top-1 accuracy: 69.8

Even though the different thing between the two conditions is ON/OFF of Conv+Relu fusion, the accuracy drop is severe. However, except for the asymmetric case, the fusion slightly improves total accuracy as you mentioned. Therefore, I added the code to prevent fusion in the case of the asymmetric schema.
Currently, I am not sure that the poor accuracy stems from fusion or there might be something that is the wrong implementation in the OpenCL backend.

In your code, I wonder if the force conversion doesn't matter.

[mciprian13]

@leejaymin The OpenCL implementation is obviously wrong:

glow/lib/Backends/OpenCL/kernels.cl

Line 1181 in 7fa1007

max(dest[getNHWC(odim, n, ax, ay, d)], (cl_int8_t)0);

The max should be done with destOffset and not 0 to capture the general case for asymmetric quantization.

And even if OpenCL likes symmetric only, best way to add fusion restrictions is to add them in the function supportsFusedActivation from OpenCL.h since it is OpenCL specific and not in GraphOptimizer which is used by all backends.

[mciprian13]

@leejaymin Could you try and compute again the accuracy? I fixed the OpenCL kernel by replacing 0 with destOffset.

[leejaymin]

@mciprian13 Thank you for your quick reply.
I pulled your PR(#4971) and ran it on CPU and OpenCL backends.
Even if you fixed the openCL code, the accuracy still shows the poor result as below.

# asymmetric case is still wrong
Finished image index 100 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 0.00
        Top-5 accuracy: 0.00
00:00:08.00
Finished image index 200 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 0.00
        Top-5 accuracy: 0.00

# symmetric case is okay
Finished image index 100 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 95.00
        Top-5 accuracy: 98.00
00:00:08.89
Finished image index 200 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 82.50
        Top-5 accuracy: 97.50

In CPU backend case, the fusion of the asymmetric case in your PR shows the right result.

# asymmetric case
Finished image index 100 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 94.00
        Top-5 accuracy: 97.00
00:00:44.30
Finished image index 200 out of 50000
  Current Top-1/5 accuracy:
        Top-1 accuracy: 80.50
        Top-5 accuracy: 97.00

In light of the outcome, there is extra something wrong in the OpenCL backend.
Thank you for your clarification and guidance.
I hope your code is getting landed soon.

[mciprian13]

@leejaymin Here is a doc describing why the math still works:
Glow_Quantization.docx

[mciprian13]

@jfix71 @jackm321 Is one of you available to review this? Thanks!

[mciprian13]

@842974287 Can you review this? Thanks!

[jfix71]

Cool @mciprian13 -- I am curious if you measured differences in perf here. Previously we were only ever fusing stacked elementwise ops IIRC. CC: @opti-mix

[mciprian13]

@jfix71 I measured the performance difference when compiling and running a floating-point MobileNet v1 0.25 128 (which is full of Clip nodes) on a ARM Cortex M7 core @ 600 MHz. The performance without fused activations was 212 ms while the performance with fused activations is 183 ms (I would say a pretty significant gain).

[mciprian13]

@jfix71 Any other items you want me to address?

[jfix71]

Nice speedup!

I've added a few suggestions inside supportsFusedActivation() -- I'd really prefer to avoid using getNthResult() unless absolutely necessary. Otherwise I think LG.

[mciprian13]

@jfix71 Review items are done. Let's have this landed then 😄

[jfix71]

Thanks!

[facebook-github-bot]

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

[facebook-github-bot]

@jfix71 merged this pull request in 923cf57.

[jfix71]

@mciprian13 FYI -- we saw significant perf degradation after this diff due to the CPU specialization for CPUConvDKKC8Node no longer applying. I'm wondering if you tested on our default CPU backend here, i.e. with CPUConvDKKC8Node included? I'm guessing you disabled this conversion when testing on an ARM core since I believe it was written somewhat specifically to an x86 core. Anyway, I think we should probably disable this fusion on the CPU backend until we can add the fusion support to the CPUConvDKKC8 kernel so that we don't see this perf degradation. CC: @hl475

[mciprian13]

@jfix71 The performance of the CPUConvDKKC8Node on ARM is just awful. For small/medium sized models the default kernel is much better. Since the CPUConvDKKC8Node was written for x86 best approach would be to disable activation fusion and enable specialization to CPUConvDKKC8Node only for x86. For this we can use the backend's getCPU function to get the CPU name and do these 2 things conditionally.