ROCmSoftwarePlatform
Forbid inefficient TensorDescriptor initialization
Initializing TensorDescriptor from std::vector<int> is very inefficient due to extra checks and multiple intermediate vector, since internally std::vector<size_t> is used.
Changed all the initializations to the native size_t, removed constructors with std::vector<int> and added workarounds for a legacy descriptors initializations with int's.
It increased performance for the current RNN implementation for a few percents.
@randyspauldingamd I believe this is now your PR :smile:. Does it pass the CI?
@randyspauldingamd I believe this is now your PR 😄. Does it pass the CI?
It doesn't.
The tests, especially old ctests, heavily rely on that int and that's why I've put the comment here https://github.com/ROCm/MIOpen/pull/3424#discussion_r1869914175 - we must not bring the old legacy code into the GTests and there is no excuses (@Vsevolod1983, @BrianHarrisonAMD, could you please check this for any upcoming PRs?)
Fixing old the CTests is a pain in the everywhere. First of all, the template code in the test driver should be adapted (hard but manageable), next - mostly all the ctests should be modified (easy but lots of changes). Good news is - I've already updated various test data configs to be compatible with both int and size_t here https://github.com/ROCm/MIOpen/pull/3416/files#diff-1daaa3cdf3d99199e44b03aea4f80477db07637a62713338f21dd1eec71e8fcd
But probably we can get rid of the old tests soon, so the problem disappeared itself.
I just had to open my big mouth...lol. Yeah, I can take this one. I expect I'll get a few of those ctest->gtest conversion tasks after the new year anyway :)
MIOpen is moving to the new monorepo setup and all older unmerged PR's are being closed. Please re-open this as part of the new repo if these changes are still needed.
