-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[GPU] Fix modnet_webcam_portrait_matting low similarity. #33370
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
[GPU] Fix modnet_webcam_portrait_matting low similarity. #33370
Conversation
Regression from openvinotoolkit#32236 Signed-off-by: hyunback <[email protected]>
8ecdb78 to
d2e6e94
Compare
| (fmt_prev == format::b_fs_yx_fsv4 && | ||
| prev_output_layout.feature() % 32 == 0 && | ||
| prev_output_layout.spatial(0) == 1 && | ||
| prev_output_layout.spatial(1) == 1)) && is_input_reorder(prev, next)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random spot) the PR description is difficult to understand. Let's discuss offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- could you improve the PR description? I think the problem itself(problematic pattern) is not described enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| (fmt_prev == format::b_fs_yx_fsv4 && | ||
| prev_output_layout.feature() % 32 == 0 && | ||
| prev_output_layout.spatial(0) == 1 && | ||
| prev_output_layout.spatial(1) == 1)) && is_input_reorder(prev, next)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- could you improve the PR description? I think the problem itself(problematic pattern) is not described enough.
| if (fmt_next == format::b_fs_yx_fsv16 || fmt_next == format::b_fs_zyx_fsv16 || | ||
| fmt_next == format::bs_fs_yx_bsv16_fsv16 || fmt_next == format::b_fs_yx_fsv4) | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm not following the logic here. If cross-layout quantization is executed, performance drops. Then I think it is better to disable cross-layout. Does OneDNN concat selection happen because of the quantization behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the different graph because of quantize and reorder fusion causes to run onednn concat. onednn concat bad accuracy is not fixed yet.
I think this PR is for restoring refactoring mistake , so It seems better to proceed separately if need. Honestly, Perf improvement shows only for this model(so special) and basically leaving the reorder seems not good.
isanghao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Regression from #32236 There are some refactoring code in #32236 (Too many if-else branches with no rules in reorder fusion function, probably just added by many people over many times, so refactoring did clean up by primitive based) But in refactoring, there are some mistake in quantize, it caused to change in reorder-fusion rule. - Previous refactoring: Do reorder fusion for quantize and reorder pattern - Refactoring Do reorder fusion for quantize and reorder pattern only quantize' s input format is plain. Because this reason, the graph has been changed (Reorder is not fused now) and it causes unintentional use of onednn concat. It occurs the poor similarity issue. This PR restores previous reorder-fusion pattern which was before refactoring so address the low similarity. And separate from the issue resolution, the onednn concat accuracy problem which was discovered accidentally due to the use of onednn concat, was issued by [MFDNN-14504 ](https://jira.devtools.intel.com/browse/MFDNN-14504) ### Tickets: - *178661* Signed-off-by: hyunback <[email protected]>
Regression from #32236
There are some refactoring code in #32236
(Too many if-else branches with no rules in reorder fusion function, probably just added by many people over many times, so refactoring did clean up by primitive based)
But in refactoring, there are some mistake in quantize, it caused to change in reorder-fusion rule.
Do reorder fusion for quantize and reorder pattern
Do reorder fusion for quantize and reorder pattern only quantize' s input format is plain.
Because this reason, the graph has been changed (Reorder is not fused now) and it causes unintentional use of onednn concat. It occurs the poor similarity issue.
This PR restores previous reorder-fusion pattern which was before refactoring so address the low similarity. And separate from the issue resolution, the onednn concat accuracy problem which was discovered accidentally due to the use of onednn concat, was issued by MFDNN-14504
Tickets: