Skip to content

Bug fixes for universal intrinsics of RISC-V back-end: v_reduce_sum.#20598

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
hanliutong:rvv-fix
Aug 26, 2021
Merged

Bug fixes for universal intrinsics of RISC-V back-end: v_reduce_sum.#20598
opencv-pushbot merged 1 commit intoopencv:masterfrom
hanliutong:rvv-fix

Conversation

@hanliutong
Copy link
Copy Markdown
Contributor

@hanliutong hanliutong commented Aug 24, 2021

Fixed reduce_sum operations.

See #20278 for previous state

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@hanliutong
Copy link
Copy Markdown
Contributor Author

cc @joy2myself and @asmorkalov

@asmorkalov asmorkalov self-requested a review August 24, 2021 07:00
@asmorkalov
Copy link
Copy Markdown
Contributor

Remaining failed test on RISC-V:

[  FAILED  ] 13 tests, listed below:
[  FAILED  ] Reproducibility_FCN.Accuracy
[  FAILED  ] Layer_GRU_Test_Accuracy_with_.Pytorch
[  FAILED  ] Test_Int8_nets.FasterRCNN_inceptionv2/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_nets.FasterRCNN_vgg16/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_nets.YOLOv3/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_ONNX_layers.LSTM_Activations/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_ONNX_layers.GRU/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_ONNX_layers.GRU_bidirectional/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_ONNX_layers.CumSum/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.reduce_max/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.reduce_max_channel/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.reduce_max_channel_keep_dims/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.pooling_reduce_max/0, where GetParam() = OCV/CPU

@hanliutong
Copy link
Copy Markdown
Contributor Author

@asmorkalov Thanks for your information! I can reproduce 4/13 failed test cases as below.

[  FAILED  ] Reproducibility_FCN.Accuracy
[  FAILED  ] Test_Int8_nets.FasterRCNN_inceptionv2/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_nets.FasterRCNN_vgg16/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_nets.YOLOv3/0, where GetParam() = OCV/CPU

And then, I test the 4 failed cases on master, both of them failed too. So I think it may not be caused by this PR.

Others 9 test cases are pass on my server but I notice that all of those test cases are added to opencv_extra in the last several weeks. So if I guess right, update the opencv_extra repository to the current master can resolve the problem.

@asmorkalov
Copy link
Copy Markdown
Contributor

Tests Test_Int8_nets_XXX are the new thing introduced in OpenCV week ago or so. Please ignore it for now. It looks like I have outdated extra or some inconsistency with my Docker containers.

@asmorkalov
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov added the pr: needs test New functionality requires minimal tests set label Aug 25, 2021
@hanliutong
Copy link
Copy Markdown
Contributor Author

Oh, I found that there is already a function to check the result of reduce_sum in "test_intrin_utils.hpp". And the reason of why we miss it before is we usually run the test with VLEN=128 on QEMU.

We can just run qemu-riscv64 -cpu rv64,x-v=true,vlen=256 ./bin/opencv_test_core --gtest_filter="hal_intrin128.float32x4_BASELINE" to reproduces the issue.

The google test report the Failure on line 1467 as below

/root/opencv/modules/core/test/test_intrin_utils.hpp:231: Failure
Expected equality of these values:
  a
    Which is: 10
  b
    Which is: 14.4375
Google Test trace:
/root/opencv/modules/core/test/test_intrin_utils.hpp:1467: i=0

Do you think we need new test cases? Or we just need a new way (with VLEN>128, VLEN=256 as an example) to run the existing tests?

@hanliutong
Copy link
Copy Markdown
Contributor Author

Unfortunately, I also found that this PR does not fix all the failures in hal_intrin128.float32x4_BASELINE. It does fix the v_reduce_sum4 but looks like there are other errors on v_rotate_right as the report by google test.

@asmorkalov
Copy link
Copy Markdown
Contributor

No new test case is not required then. I'll add vlen=256 case to CI.

@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Aug 26, 2021
@jebastin-nadar
Copy link
Copy Markdown
Contributor

Hi @asmorkalov @hanliutong. Can you please specify what is the exact error in the failed Test_Int8_Nets.XXX? Is it related to stricter thresholds for scoreDiff and IoUDiff?

@opencv-pushbot opencv-pushbot merged commit 56d0d59 into opencv:master Aug 26, 2021
@alalek alalek mentioned this pull request Oct 15, 2021
@hanliutong hanliutong deleted the rvv-fix branch November 9, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants