[NFC] Replace miopen::ProblemDescription with conv::ProblemDescription, part 3 #2303
Conversation
averinevg
commented
Aug 7, 2023
averinevg
commented
- Make miopen::ProblemDescription derived from conv::ProblemDescription
- miopen::ProblemDescription: remove getters that exist in the parent class
- conv::ProblemDescription: use proper data type of return value for getters that use data from TensorDescriptor
- conv::ProblemDescription: add trailing underscores to getters with changed data type in order to get the attention of developers who are not familiar with these changes (will be removed after ROCm 6.0)
- Remove unused code
…problem_description_part_3
…problem_description_part_3
…problem_description_part_3
…problem_description_part_3
…problem_description_part_3_1
…problem_description_part_3_1
…problem_description_part_3_1
…problem_description_part_3_1
…problem_description_part_3_2
…problem_description_part_3_2
| @@ -285,7 +285,7 @@ struct TensorDescriptor : miopenTensorDescriptor | |||
| }; | |||
|
|
|||
| template <class TElement> | |||
| constexpr auto GetNCDHW(int spatial_dims, const std::vector<TElement>& data) | |||
| constexpr auto GetNCDHW(unsigned spatial_dims, const std::vector<TElement>& data) | |||
There was a problem hiding this comment.
[Q] why this is necessary?
There was a problem hiding this comment.
This variable is always positive and using a signed data type can be misleading
There was a problem hiding this comment.
That is, you just caught a not quite high-quality code and decided to fix it, right? 😄
There was a problem hiding this comment.
Yes, you are right 😄. I decided to change this too, because it is related to my changes
| unsigned GetInBatchSize_() const { return GetN5(GetSpatialDims(), in.GetLengths()); } | ||
| unsigned GetBatchSize_() const { return GetInBatchSize_(); } // alias of GetInBatchSize_() | ||
| unsigned GetInChannels_() const { return GetC5(GetSpatialDims(), in.GetLengths()); } | ||
| unsigned GetInDepth_() const { return GetD5(GetSpatialDims(), in.GetLengths()); } | ||
| unsigned GetInHeight_() const { return GetH5(GetSpatialDims(), in.GetLengths()); } | ||
| unsigned GetInWidth_() const { return GetW5(GetSpatialDims(), in.GetLengths()); } | ||
| unsigned GetInBatchStride_() const { return GetN5(GetSpatialDims(), in.GetStrides()); } | ||
| unsigned GetInChannelStride_() const { return GetC5(GetSpatialDims(), in.GetStrides()); } | ||
| unsigned GetInStrideD_() const { return GetD5(GetSpatialDims(), in.GetStrides()); } | ||
| unsigned GetInStrideH_() const { return GetH5(GetSpatialDims(), in.GetStrides()); } | ||
| unsigned GetInStrideW_() const { return GetW5(GetSpatialDims(), in.GetStrides()); } |
There was a problem hiding this comment.
[Q] I am not sure this is a good change. Let's discuss.
There was a problem hiding this comment.
I would also like to know why we went to unsigned from size_t and why the names have an _ at the end of getters.
There was a problem hiding this comment.
@amberhassaan You can find details here #2303 (comment) and here #2303 (comment)
In most places in the library miopen::ProblemDescription is used, its getters returned the int type, the getters of conv::ProblemDescription returned the size_t type. For now, it is enough for us to have int. unsigned is used because we don't need a sign.
Yes, this is a temporary change |
|
WRT #2303 (comment). After ~2 hours discussion we decided to continue as is and then switch to size_t in a dedicated PR. |
| const auto W = (problem).GetInWidth(); \ | ||
| const auto out_H = (problem).GetOutHeight(); \ | ||
| const auto out_W = (problem).GetOutWidth(); | ||
| #define DEFINE_SHADER_ALIASES(problem) \ |
There was a problem hiding this comment.
An idea for future work to ease solver developers life:
Similar code is verbose and present in most of our solvers. We should add some method like CommonlyUsedConvParameters GetCommonlyUsedConvParameters with CommonlyUsedConvParameters being some type which is compatible to structured bindings so it would be possible to either extract them as const auto [grop_cnt, N, K, ...] = problem.GetCommonlyUsedConvParameters() or just as const auto params = GetCommonlyUsedConvParameters() to use as params.group_cnt and such.
There was a problem hiding this comment.
Perhaps we should also refactor other "helpers" that get data from ProblemDescription. I assume that they partially overlap and can be generalized.
…problem_description_part_3_2
…problem_description_part_3_2
…problem_description_part_3_2
…problem_description_part_3_2
…problem_description_part_3_2
|
@junliume We can merge this when you get a chance |
| @@ -38,6 +38,8 @@ | |||
| #include <cstdint> | |||
| #include <string> | |||
|
|
|||
| #define FIN_OLD_PROBLEM_DESCRIPTION_COMPAT 1 | |||
There was a problem hiding this comment.
Is this flag documented somewhere?
There was a problem hiding this comment.
No, it is supposed to be used with MIFin for backward compatibility and will be removed soon.
averinevg
deleted the
ea_replace_problem_description_with_conv_problem_description_part_3_2
branch
