Skip to content

MP_bidirect_winograd with xdlops [FP32]#454

Merged
daniellowell merged 13 commits intodevelopfrom
MP_BD_winograd_xdlops_PR
Oct 6, 2020
Merged

MP_bidirect_winograd with xdlops [FP32]#454
daniellowell merged 13 commits intodevelopfrom
MP_BD_winograd_xdlops_PR

Conversation

@shurale-nkn
Copy link
Copy Markdown
Contributor

@shurale-nkn shurale-nkn commented Sep 22, 2020

Added new MP_bidirect_winograd solver with xdlops support.
This solver is tunable.

gfx908 time improvements without new solver tuning

N-C-K-H*W-x*y-p*q-u*v direction time reduce acceleration develop, best algo this PR
256-128-128-28*28-3*3-1*1-1*1 FWD 15% 18% ConvMPBidirectWinograd<6-3> ConvMPBidirectWinograd_xdlops<6-3>
256-128-128-28*28-3*3-1*1-1*1 BWD 15% 18% ConvMPBidirectWinograd<6-3> ConvMPBidirectWinograd_xdlops<6-3>
256-256-256-14*14-3*3-1*1-1*1 FWD 39% 64% ConvBinWinogradRxSf2x3 ConvMPBidirectWinograd_xdlops<5-3>
256-256-256-14*14-3*3-1*1-1*1 BWD 38% 61% ConvBinWinograd3x3U ConvMPBidirectWinograd_xdlops<5-3>
256-512-512-7*7-3*3-1*1-1*1 FWD 54% 117% ConvMPBidirectWinograd<6-3> ConvMPBidirectWinograd_xdlops<4-3>
256-512-512-7*7-3*3-1*1-1*1 BWD 51% 104% ConvMPBidirectWinograd<6-3> ConvMPBidirectWinograd_xdlops<4-3>

@daniellowell
Copy link
Copy Markdown

@shurale-nkn Can you share target configs and performance data?

@shurale-nkn
Copy link
Copy Markdown
Contributor Author

shurale-nkn commented Sep 24, 2020

@shurale-nkn Can you share target configs and performance data?

@daniellowell
If you need more data, I can create a private thread.

@atamazov
Copy link
Copy Markdown
Contributor

@shurale-nkn

If you need more data, I can create a private thread.

Better open an ticket for discussions at https://github.com/AMDComputeLibraries/MLOpen

Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/include/miopen/solver.hpp
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp
removed RunAndMeasureSolution from ConvMPBidirectWinograd_xdlops solver
ConvMPBidirectWinograd_xdlops<2-3> returned
@asroy
Copy link
Copy Markdown
Contributor

asroy commented Sep 28, 2020

@shurale-nkn Has rocblas been tuned for these configs?

Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp Outdated
Comment thread src/include/miopen/solver.hpp
Copy link
Copy Markdown
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

Minor issues

@shurale-nkn
Copy link
Copy Markdown
Contributor Author

@shurale-nkn Has rocblas been tuned for these configs?

@asroy
If we speak about ConvMPBidirectWinograd algo in develop, then no, because ticket for tuning created a week ago. They wouldn't have had time to do it.

If about ConvMPBidirectWinograd_xdlops, than no, ConvHipImplicitGemmForwardV4R4Xdlops not tuned, used config from EuristicInit .

Copy link
Copy Markdown
Contributor

@Kirpich30000 Kirpich30000 left a comment

Choose a reason for hiding this comment

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

minor changes

Comment thread src/kernels/xform_bidirect_winograd_code.inc Outdated
@daniellowell daniellowell changed the title MP_bidirect_winograd with xdlops MP_bidirect_winograd with xdlops [FP32] Sep 29, 2020
Kirpich30000
Kirpich30000 previously approved these changes Oct 1, 2020
Copy link
Copy Markdown
Contributor

@Kirpich30000 Kirpich30000 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 2, 2020

Codecov Report

Merging #454 into develop will decrease coverage by 0.07%.
The diff coverage is 25.86%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #454      +/-   ##
===========================================
- Coverage    53.24%   53.16%   -0.08%     
===========================================
  Files          296      296              
  Lines        44986    45096     +110     
===========================================
+ Hits         23953    23976      +23     
- Misses       21033    21120      +87     
Impacted Files Coverage Δ
src/include/miopen/solver.hpp 15.26% <0.00%> (-0.62%) ⬇️
src/solver/conv_MP_bidirectional_winograd.cpp 18.86% <20.65%> (+8.34%) ⬆️
src/mlo_dir_conv.cpp 88.52% <100.00%> (ø)
src/solver.cpp 91.62% <100.00%> (+0.43%) ⬆️
src/md5.cpp 90.57% <0.00%> (-2.90%) ⬇️
src/ocl/clhelper.cpp 86.66% <0.00%> (-1.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26b60d3...4eef1a1. Read the comment docs.

@daniellowell
Copy link
Copy Markdown

@shurale-nkn Merge conflicts and blocking review. Please update this ASAP, otherwise it will not get into ROCm 3.10.

@shurale-nkn shurale-nkn requested a review from atamazov October 5, 2020 16:57
Copy link
Copy Markdown
Contributor

@Kirpich30000 Kirpich30000 left a comment

Choose a reason for hiding this comment

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

LGTM

@daniellowell daniellowell added this to the ROCm 3.10 milestone Oct 5, 2020
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp
Comment thread src/solver/conv_MP_bidirectional_winograd.cpp
Copy link
Copy Markdown
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

Good job of reusing existing kernels/solvers
makeitso

@atamazov
Copy link
Copy Markdown
Contributor

atamazov commented Oct 5, 2020

Please do not forget to document MIOPEN_DEBUG_AMD_MP_BD_WINOGRAD_WORKSPACE_MAX in separate PR. Also it would be nice to get rid of // cppcheck-suppress unreadVariable for WinoOffsets.

@shurale-nkn
Copy link
Copy Markdown
Contributor Author

@daniellowell
Update! Git doesn't display this, but the testing was completed successfully.

@daniellowell daniellowell merged commit fcf4ed8 into develop Oct 6, 2020
daniellowell pushed a commit that referenced this pull request Oct 18, 2020
* add xdlops_winograd_solver

* format

* tidy fix

* small code fix
removed RunAndMeasureSolution from ConvMPBidirectWinograd_xdlops solver

* Multi-pass Winograd documentation updated
ConvMPBidirectWinograd_xdlops<2-3> returned
* WinoOffsets cppcheck-suppress unreadVariable
JehandadKhan pushed a commit that referenced this pull request Nov 20, 2020
* add xdlops_winograd_solver

* format

* tidy fix

* small code fix
removed RunAndMeasureSolution from ConvMPBidirectWinograd_xdlops solver

* Multi-pass Winograd documentation updated
ConvMPBidirectWinograd_xdlops<2-3> returned
* WinoOffsets cppcheck-suppress unreadVariable
@shurale-nkn shurale-nkn deleted the MP_BD_winograd_xdlops_PR branch February 2, 2021 16:00
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