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

Enable NCHW/NHWC and NCDHW/NDHWC layout in batch norm driver command #3234

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

bghimireamd
Copy link
Contributor

@bghimireamd bghimireamd commented Sep 4, 2024

  • Enable NCHW/NHWC layout from driver command for batch norm.
  • Moved GpumemTensor to driver/driver.hpp
  • Stopped using old and slow miopenBNFwdTrainPerActivationRunHost, miopenBNFwdTrainSpatialRunHost, miopenBNFwdInferPerActivationRunHost, miopenBNFwdInferSpatialRunHost, miopenBNBwdPerActivationRunHost and miopenBNBwdSpatialRunHost since they only handled NCHW layout. Started using layout agnostic batch norm host function from test/fusionHost.hpp

eg:

./bin/MIOpenDriver bnorm -n 64 -c 1024 -H 14 -W 14 -m 1 --forw 1 -b 0 -s 0 -r 1 // defaults to NCHW

./bin/MIOpenDriver bnorm -n 64 -c 1024 -H 14 -W 14 -m 1 --forw 1 -b 0 -s 0 -r 1 -L NCHW
./bin/MIOpenDriver bnorm -n 64 -c 1024 -H 14 -W 14 -m 1 --forw 1 -b 0 -s 0 -r 1 -L NHWC
./bin/MIOpenDriver bnorm -n 64 -c 1024 -H 14 -W 14 -m 1 --forw 1 -b 0 -s 0 -r 1 --layout NHWC

bnorm, bnormfp16, bnormfp16fp32, bnormbfp16fp32,

3D

./bin/MIOpenDriver bnorm -n 8 -c 8 -H 12 -W 12 -D12 -m 1 --forw 0 -b 1 -s 1 -r 1 --layout NDHWC

@bghimireamd bghimireamd changed the title Initial attempt to enable NHWC layout for batch norm driver command Enable NCHW/NHWC layout in batch norm driver command Sep 4, 2024
Copy link
Contributor

@alexandraBara alexandraBara left a comment

Choose a reason for hiding this comment

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

-L/--layout is quite generic. Which layout does it refer to? in/out/fil?
In convolution we do this: --fil_layout NCHW --in_layout NCHW --out_layout NCHW - which is nice bc its specific. I think BN should follow the same pattern.
Maybe there will be a time where in/fil layout in BN will be different, so we need to keep that in mind

Copy link
Contributor

@CAHEK7 CAHEK7 left a comment

Choose a reason for hiding this comment

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

Some minor suggestions

}

template <typename T>
status_t AllocOnDevice(stream, context_t ctx, const size_t sz, std::vector<T>&)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's suspicious:

status_t AllocOnDevice(stream, context_t ctx, const size_t sz)

I'm not sure that we want another copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I bring class GpumemTensor from conv_driver.hpp into another file driver_common.hpp and let both conv_driver.hpp and bn_driver.hpp use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and somewhen, in the future, when you will see any raw or even managed gpu pointers in the driver\tests, you can recommend to use that class.

Copy link
Contributor Author

@bghimireamd bghimireamd Sep 25, 2024

Choose a reason for hiding this comment

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

For now I moved to driver.hpp

driver/bn_driver.hpp Outdated Show resolved Hide resolved
@bghimireamd bghimireamd marked this pull request as ready for review September 6, 2024 02:01
@bghimireamd bghimireamd marked this pull request as draft September 6, 2024 02:44
@bghimireamd bghimireamd marked this pull request as ready for review September 25, 2024 13:52
@bghimireamd bghimireamd changed the title Enable NCHW/NHWC layout in batch norm driver command Enable NCHW/NHWC and NCDHW/NDHWC layout in batch norm driver command Sep 26, 2024
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.

4 participants