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

Reference kernel for 3D convolution for non-packed tensors #2334

Merged
merged 13 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ if( MIOPEN_BACKEND MATCHES "OpenCL" OR MIOPEN_BACKEND STREQUAL "HIPOC" OR MIOPEN
kernels/workaround_issue_1431.hpp
kernels/hip_f8_impl.hpp
kernels/hip_float8.hpp
kernels/stride_array.hpp
)

set(MIOPEN_KERNELS
Expand Down
2 changes: 1 addition & 1 deletion src/hip/hip_build_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ static boost::filesystem::path HipBuildImpl(boost::optional<TmpDir>& tmp_dir,
auto env = std::string("");

if(params.find("-std=") == std::string::npos)
params += " --std=c++11";
params += " --std=c++17";
Copy link
Contributor

Choose a reason for hiding this comment

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

This affects all the HIP kernels included into the library (that do not explicitly specify C++ standard) and therefore requires extensive testing (and should be done in a separate PR at least). Please revert this and produce the necessary option in the naive solvers.


#if HIP_PACKAGE_VERSION_FLAT < 4001000000ULL
params += " --cuda-gpu-arch=" + lots.device;
Expand Down
24 changes: 8 additions & 16 deletions src/include/miopen/hipoc_kernel.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@
#ifndef GUARD_MIOPEN_HIPOC_KERNEL_HPP
#define GUARD_MIOPEN_HIPOC_KERNEL_HPP

#include <array>
#include <cassert>
#include <miopen/errors.hpp>
#include <miopen/hipoc_program.hpp>
#include <miopen/stringutils.hpp>
#include <miopen/op_kernel_args.hpp>

#include <array>
#include <cassert>
#include <cstring>
#include <vector>
#include <memory.h>

namespace miopen {

Expand All @@ -47,29 +48,20 @@ inline HipEventPtr make_hip_event()

#if 1 // Keep around other storage techinques -- @pfultz2 27.03.2017

#if 1 // Keep around other storage techinques -- @pfultz2 27.03.2017
Copy link
Contributor

Choose a reason for hiding this comment

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

[R] I am not sure that we want to remove this. In my previous PR I decided to keep this as an illustration of the original idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original had a bug on line 54. The new code is the same as original except that line 54 is fixed. I don't think there's any benefit in keeping buggy code (rather may cause harm if someone assumes that it works).

Copy link
Contributor

@atamazov atamazov Sep 28, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any value in keeping this code. It's just a simple pair class. We have always needed proper alignment for kernel arguments, so keeping around code that doesn't address alignment issues is just misguiding and confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets clean up the code, if we ever need it I am sure its not too difficult to write it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

@amberhassaan This comment is just a recommendation. You are free to either follow recommendations or deny them, no explanations needed.

so keeping around code that doesn't address alignment issues is just misguiding and confusing.

Thanks for positive feedback.

template <class T, class U>
struct KernelArgsPair
{
static const int alignment = sizeof(U);
static const int padding = (alignment - sizeof(T) % alignment) % alignment;
static const int second_index = sizeof(T) + padding;
constexpr static auto alignU = alignof(U);
constexpr static auto padding = (alignU - (sizeof(T) % alignU)) % alignU;
constexpr static auto second_index = sizeof(T) + padding;
KernelArgsPair(T x, U y)
{
new(buffer) T(x); // NOLINT (clang-analyzer-cplusplus.PlacementNew)
new(buffer + second_index) U(y);
}

alignas(U) char buffer[second_index + sizeof(U)] = {};
};
#else
averinevg marked this conversation as resolved.
Show resolved Hide resolved
template <class T, class U>
struct KernelArgsPair
{
KernelArgsPair(T x, U y) : first(x), second(y) {}
T first;
U second;
};
#endif

template <class... Ts>
struct KernelArgsPack;
Expand Down
95 changes: 94 additions & 1 deletion src/include/miopen/solver/conv_direct_naive_conv.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@
*******************************************************************************/
#pragma once

#include <string>
#include <miopen/execution_context.hpp>
#include <miopen/problem_description.hpp>
#include "miopen/../../kernels/stride_array.hpp"

#include <array>
#include <algorithm>
#include <cassert>
#include <string>
#include <vector>

namespace miopen {

Expand All @@ -54,5 +60,92 @@ bool IsOutputBfp16(const ProblemDescription&);
bool IsOutputInt8(const ProblemDescription&);
bool IsOutputInt32(const ProblemDescription&);

namespace conv_internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

[Recommendation] conv_naive

This is continuation of #2334 (comment)


void DebugPrintTensorStrides(const TensorDescriptor& inDesc,
const TensorDescriptor& wDesc,
const TensorDescriptor& outDesc);

/**
* Get the index where group (G) stride should go. For NCHW, we want to convert
* its strides to NGCHW, and for NHWC, we want to convert its strides to NHWGC.
* Same applies for the 3D case.
*/
int GetGroupStrideIndex(const ProblemDescription& problem);

/**
* split the strides for C dimension in a tensor descriptor into (G, C_per_group).
* Normally, (in packed case) num channels is a multiplying factor in the stride of
* whatever lies to the left of C, e.g., in NCHW, N's stride contains C as a
* factor. We output NGCHW for NCHW (and NHWGC for NHWC)
* where the stride[G] = stride[N] / num_groups
*/
template <typename V>
V SplitStrideCtoGC(int num_groups, const V& orig_strides, int G_stride_idx)
{
assert(G_stride_idx > 0 && G_stride_idx <= orig_strides.size());
// (G_stride_idx - 1) is the stride index of whatever lies to the left and
// contains C or K as a multiplying factor. We divide this value by num_groups
// to get G_stride_val
assert(orig_strides[G_stride_idx - 1] % num_groups == 0);

V ret{orig_strides};
auto G_stride_val = orig_strides[G_stride_idx - 1] / num_groups;

ret.insert(ret.begin() + G_stride_idx, G_stride_val);

return ret;
}

/**
* Weight tensor has original dims: [K, C_per_group, Y, X] (2D case)
* We return a new stride vector with strides for [G, K_per_group, C_per_group, Y, X]
* Stride for G is computed as stride[C_per_group] * K_per_group and inserted at
* left most position
*/
template <typename V>
V SplitWeiStrideKtoGK(int k_per_group, const V& wei_strides)
{
V ret{wei_strides};
ret.insert(ret.begin(), wei_strides[0] * k_per_group);
return ret;
}

template <unsigned N>
struct ChooseStride
{
};

template <>
struct ChooseStride<5u>
{
using type = Strides5D;
};

template <>
struct ChooseStride<6u>
{
using type = Strides6D;
};

template <unsigned N, typename V>
auto MakeStrideArray(V vec)
{
typename ChooseStride<N>::type ret;
assert(vec.size() == N);

// MIOpen stores strides for NHWC in NCHW order, i.e. C stride in 2nd from left.
// We sort the input stride vector so that smallest stride is at index 0. This
// (little-endian) order is what naive convolution kernel expects for strides
std::sort(vec.begin(), vec.end());

for(unsigned i = 0; i < N; ++i)
{
ret[i] = static_cast<StrideIndexType>(vec[i]);
}
return ret;
}
} // end namespace conv_internal

} // namespace solver
} // namespace miopen
6 changes: 3 additions & 3 deletions src/kernels/gpu_reference_kernel/fp8_kern_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,6 @@

#define KERNEL_NAME_SUFFIX CAT(CAT(INPUT_TYPE, _), CAT(CAT(WEIGHTS_TYPE, _), OUTPUT_TYPE))

#define FWD_KERNEL_NAME CAT(naive_conv_fwd_nchw_, KERNEL_NAME_SUFFIX)
#define BWD_KERNEL_NAME CAT(naive_conv_bwd_nchw_, KERNEL_NAME_SUFFIX)
#define WRW_KERNEL_NAME CAT(naive_conv_wrw_nchw_, KERNEL_NAME_SUFFIX)
#define FWD_KERNEL_NAME CAT(naive_conv_packed_fwd_nchw_, KERNEL_NAME_SUFFIX)
#define BWD_KERNEL_NAME CAT(naive_conv_packed_bwd_nchw_, KERNEL_NAME_SUFFIX)
#define WRW_KERNEL_NAME CAT(naive_conv_packed_wrw_nchw_, KERNEL_NAME_SUFFIX)
Loading