Skip to content
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

DNN graph fusion with shared nodes #24463

Merged
merged 6 commits into from Nov 3, 2023

Conversation

dkurt
Copy link
Member

@dkurt dkurt commented Oct 27, 2023

Pull Request Readiness Checklist

For now, nodes from matched pattern are removed during the matching process so if nodes are used in similar subgraph, they cannot be found.

required for #24397

Merge with extra: opencv/opencv_extra#1115

A part from model_name with two Resize subgraphs with shared nodes:
image

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

@fengyuentau
Copy link
Member

Two questions come to my mind:

  1. We do not have test cases for layer simplification (or fusion) yet. How do we know the changes in modules/dnn/src/graph_simplifier.cpp will not downgrade current performance, except thinking real hard in our brain?
  2. Since this shared constant node issue is brought up, what about the case where a single Const node is used in several nodes, some of which can be fused (thus this single Const node is removed), while the others of which still need this Const node for input?

@dkurt
Copy link
Member Author

dkurt commented Oct 27, 2023

@fengyuentau, let me try to answer your questions:

  1. We do not have test cases for layer simplification (or fusion) yet. How do we know the changes in modules/dnn/src/graph_simplifier.cpp will not downgrade current performance, except thinking real hard in our brain?

For now all the tests relies that fusion is enabled and we do not check what will happen if fusion disabled. Probably, some of the tests will still work thanks to improvements in ONNX. And there would be a performance questions, that's right.

  1. Since this shared constant node issue is brought up, what about the case where a single Const node is used in several nodes, some of which can be fused (thus this single Const node is removed), while the others of which still need this Const node for input?

Good point. Moreover, it's not only about Const but any type of nodes. For example, Shape which can be obtained once but reused in different subgraphs. Let's switch this PR to draft so I can try alternative solutions with consumers counting.

@dkurt dkurt marked this pull request as draft October 27, 2023 14:31
@fengyuentau
Copy link
Member

fengyuentau commented Oct 27, 2023

Also worth mentioning is that some operators, such as Add and Mul, conform to commutative law (swapping input A and B does not yield different result). Sometimes the Const input can be input A, and the other times input B. Current graph simplifier cannot deal with this case unless we write down all the possible patterns. If there are N Add or Mul operators in a subgraph and they have one of the input to be constant, the combination would be 2^N if we want to match all possible subgraphs.

I have studied your code in graph_simplifier.cpp, especially match, but did not come up with a good solution without making a lot changes.

@fengyuentau
Copy link
Member

For now all the tests relies that fusion is enabled and we do not check what will happen if fusion disabled. Probably, some of the tests will still work thanks to improvements in ONNX. And there would be a performance questions, that's right.

The performance part can be tricky, but at least we can add some fusion tests checking whether the net is simplified to the target form? For example, in most of the cases, if we provide the exact subgraph to be fused, the simplified net should have only one node with matching node type.

@dkurt dkurt marked this pull request as ready for review October 31, 2023 11:46
@dkurt
Copy link
Member Author

dkurt commented Oct 31, 2023

@fengyuentau, I will try to solve Add/Mul commutative nodes matching in a separate PR.

Copy link
Member

@fengyuentau fengyuentau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@asmorkalov asmorkalov merged commit fa56623 into opencv:4.x Nov 3, 2023
26 checks passed
@dkurt dkurt deleted the dnn_shared_nodes_fusion branch November 7, 2023 08:03
asmorkalov pushed a commit that referenced this pull request Nov 8, 2023
Commutative rules for DNN subgraphs fusion #24483

### Pull Request Readiness Checklist

related: #24463 (comment)

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
IskXCr pushed a commit to Haosonn/opencv that referenced this pull request Dec 20, 2023
DNN graph fusion with shared nodes opencv#24463

### Pull Request Readiness Checklist

For now, nodes from matched pattern are removed during the matching process so if nodes are used in similar subgraph, they cannot be found.

required for opencv#24397

**Merge with extra**: opencv/opencv_extra#1115

A part from [model_name ](https://github.com/onnx/models/blob/main/vision/object_detection_segmentation/fcn/model/fcn-resnet101-11.onnx) with two Resize subgraphs with shared nodes:
![image](https://github.com/opencv/opencv/assets/25801568/611d89d9-12fb-4add-9218-13b10d2c086a)

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
IskXCr pushed a commit to Haosonn/opencv that referenced this pull request Dec 20, 2023
Commutative rules for DNN subgraphs fusion opencv#24483

### Pull Request Readiness Checklist

related: opencv#24463 (comment)

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
DNN graph fusion with shared nodes opencv#24463

### Pull Request Readiness Checklist

For now, nodes from matched pattern are removed during the matching process so if nodes are used in similar subgraph, they cannot be found.

required for opencv#24397

**Merge with extra**: opencv/opencv_extra#1115

A part from [model_name ](https://github.com/onnx/models/blob/main/vision/object_detection_segmentation/fcn/model/fcn-resnet101-11.onnx) with two Resize subgraphs with shared nodes:
![image](https://github.com/opencv/opencv/assets/25801568/611d89d9-12fb-4add-9218-13b10d2c086a)

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
Commutative rules for DNN subgraphs fusion opencv#24483

### Pull Request Readiness Checklist

related: opencv#24463 (comment)

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
@asmorkalov asmorkalov mentioned this pull request Jan 19, 2024
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.

None yet

4 participants