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

Backward data #246

Merged
merged 23 commits into from
Jun 7, 2021
Merged

Backward data #246

merged 23 commits into from
Jun 7, 2021

Conversation

ltqin
Copy link
Contributor

@ltqin ltqin commented May 31, 2021

This PR is backward data muti-kernels version, if merge with oob_write(#227). stride=2 case can pass

@ltqin ltqin added skip-ci Don't build Jenkins tests and removed skip-ci Don't build Jenkins tests labels May 31, 2021
@ltqin ltqin requested review from asleepzzz, whchung and asroy May 31, 2021 10:21
@whchung whchung requested review from sjw36 and jerryyin May 31, 2021 12:16
@whchung
Copy link
Contributor

whchung commented May 31, 2021

@asleepzzz / @ltqin is logic in getGemmA correct about OOB dims?

@whchung
Copy link
Contributor

whchung commented May 31, 2021

@ltqin please fix merge conflicts.

and now since #227 has been merged, it's better to do a rebase.

@whchung
Copy link
Contributor

whchung commented Jun 1, 2021

jenkins: retest this please.

Copy link
Member

@jerryyin jerryyin left a comment

Choose a reason for hiding this comment

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

Two major issues needs attention/resolution:

  • gemmId should be guaranteed to be consecutive.
  • You need to have end-to-end interface unit test to test the client perspective, since there are fairly fundamental changes. You need to add test such that it take advantage of mlir-miopen-lib-test.cpp to compile at least one case of bwd that can populate multiple kernels.

mlir/lib/Dialect/MIOpen/Generator/Conv2dGenerator.cpp Outdated Show resolved Hide resolved
mlir/tools/mlir-miopen-lib/mlir-miopen-lib.cpp Outdated Show resolved Hide resolved
@whchung whchung requested a review from jerryyin June 2, 2021 16:53
@ltqin
Copy link
Contributor Author

ltqin commented Jun 3, 2021

  • You need to have end-to-end interface unit test to test the client perspective, since there are fairly fundamental changes. You need to add test such that it take advantage of mlir-miopen-lib-test.cpp to compile at least one case of bwd that can populate multiple kernels.

@jerryyin please check unit test: mlir/test/mlir-miopen-driver/populate_bwd_multi_kernels.mlir

@jerryyin
Copy link
Member

jerryyin commented Jun 3, 2021

@whchung
Copy link
Contributor

whchung commented Jun 3, 2021

jenkins: retest this please.

@ltqin ltqin requested a review from asroy June 4, 2021 03:19
@ltqin
Copy link
Contributor Author

ltqin commented Jun 4, 2021

@asroy According information(ROCm/MIOpen#917), I test the cases that fail on MIOpen . but all case pass on mlir-miopen-driver

@ltqin
Copy link
Contributor Author

ltqin commented Jun 4, 2021

@ltqin That file is not enough because it is not end-to-end.

You'd need to add a for loop here such that it can cover multi-kernel case:

@jerryyin please check the update, there are some different from what we talked

@jerryyin
Copy link
Member

jerryyin commented Jun 4, 2021

@ltqin We synced offline, I'd prefer to reconstruct the arguments instead of forcing the client to take the kernel_id in all scenarios.

@whchung whchung requested a review from jerryyin June 5, 2021 05:12
Copy link

@asroy asroy left a comment

Choose a reason for hiding this comment

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

LGTM.

@ltqin I'm not sure about the logic for vector load, but since you have test some configs and they passed. I'm going to approve it.

Please paste the configs you have tested for future records.

Please also watch this task, I will write done the derivation for bwd-data-v4r1 when I got time, and then you can check with current implementation.
https://github.com/ROCmSoftwarePlatform/miopen-kernel-project-private/issues/45

@whchung whchung merged commit 22c4eb7 into miopen-dialect Jun 7, 2021
@whchung whchung deleted the backward_data branch June 7, 2021 16:25
@ltqin
Copy link
Contributor Author

ltqin commented Jun 8, 2021

@asroy I have tested configs for padding issue:
./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=14 -in_w=14 -fil_h=1 -fil_w=1 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c -rand 1| ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=14 -in_w=14 -fil_h=3 -fil_w=3 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c -rand 1| ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=15 -in_w=15 -fil_h=1 -fil_w=1 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c -rand 1| ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=16 -in_w=16 -fil_h=1 -fil_w=1 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c | ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=18 -in_w=18 -fil_h=1 -fil_w=1 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c -rand 1| ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

./bin/mlir-miopen-driver -p=false -fil_layout=gkcyx -in_layout=ngchw -out_layout=ngkhw -batchsize=32 -in_channels=32 -out_channels=32 -in_h=20 -in_w=20 -fil_h=1 -fil_w=1 --dilation_h=1 --dilation_w=1 --padding_h=1 --padding_w=1 --conv_stride_h=1 --conv_stride_w=1 --groupsize=1 -pv --operation=conv2d_bwd_data -c -rand 1| ./bin/mlir-rocm-runner --shared-libs=./lib/librocm-runtime-wrappers.so,./lib/libmlir_runner_utils.so --entry-point-result=void

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants